✨ (dmk) [DSDK-1074]: Add language package management#1382
✨ (dmk) [DSDK-1074]: Add language package management#1382pvautherin-ledger wants to merge 21 commits intodevelopfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
4fd753d to
735aaca
Compare
735aaca to
306576f
Compare
306576f to
26b4375
Compare
95e0f02 to
45b7bcf
Compare
b52e273 to
92610fd
Compare
4b301c4 to
c3c26a5
Compare
c3c26a5 to
e9e8e33
Compare
e9e8e33 to
dd4d8f3
Compare
dd4d8f3 to
08879fd
Compare
…progress tracking
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
d9af3af to
22023f7
Compare
|
| const execute = async () => { | ||
| const { data: rawApdus } = await axios.get<string>( | ||
| this.args.apduInstallUrl, | ||
| ); |
There was a problem hiding this comment.
[SHOULD] If this throws, it should be wrapped in a InstallLanguagePackageTaskError, especially because the DeviceAction makes this type assertion, even though it's incorrect.
|
|
||
| execute().then( | ||
| () => subscriber.complete(), | ||
| (err) => subscriber.error(err), |
There was a problem hiding this comment.
[SHOULD] I think we should avoid subscriber.error because then errors are of type unknown. What we could do instead is always emit those errors in the observable directly (it becomes Observable<ProgressEvent|ErrorEvent>), and like this it does not force the DeviceAction to assert unknown as *DAError
There was a problem hiding this comment.
[COULD] Overall I feel like the task could be much easier to follow if it was rewritten as an RxJS pipeline (with operators like defer, from, switchMap, catchError etc. instead of new Observable(() => {...}) wrapping the async logic in an inner function, throwing inside it and catching in then etc.
I think it's worth exploring the alternative to see if it would be a bit cleaner, but it's not a required change if you already tackle the [SHOULD] comments.
OlivierFreyssinet
left a comment
There was a problem hiding this comment.
Nice 👍
Just some things to rework regarding error flow in the install task.


📝 New commands ✨
New Device Action 🎉
❓ Context
JIRA: https://ledgerhq.atlassian.net/browse/DSDK-1074
Language Package Management Study: Confluence
Legacy LW Implementation: LucidChart
DMK Device Action flow : LucidChart
✅ Checklist
Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.
🧐 Checklist for the PR Reviewers