Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support/LIVE-3247 Replace usb-detection library with node-usb #903

Merged
merged 6 commits into from
Sep 8, 2022

Conversation

juan-cortes
Copy link
Contributor

πŸ“ Description

Replace usb-detection library with node-usb

❓ Context

  • Impacted projects: ledger-live-desktop
  • Linked resource(s): LIVE-3247

βœ… Checklist

  • Test coverage

  • Atomic delivery

  • No breaking changes. No breaking changes in code integration, if that's what you mean.

πŸ“Έ Demo

No demo

πŸš€ Expectations to reach

Since this is a transport change it requires manual testing from QA to assess whether we introduced regressions or not. My initial assessment is that it all works as expected but we'll need to test on other operating systems and versions too. There are some type inconsistencies between the 4yo version and this but we do not use any of the exported properties anyway.

@vercel
Copy link

vercel bot commented Aug 11, 2022

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
live-common-tools βœ… Ready (Inspect) Visit Preview Sep 6, 2022 at 4:01PM (UTC)
3 Ignored Deployments
Name Status Preview Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Sep 6, 2022 at 4:01PM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Sep 6, 2022 at 4:01PM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Sep 6, 2022 at 4:01PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2022

πŸ¦‹ Changeset detected

Latest commit: 28dd290

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@ledgerhq/hw-transport-node-hid-singleton Minor
ledger-live-desktop Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #903 (27dde8a) into develop (6edf507) will increase coverage by 47.84%.
The diff coverage is n/a.

❗ Current head 27dde8a differs from pull request most recent head 28dd290. Consider uploading reports for the commit 28dd290 to get more accurate results

@@             Coverage Diff              @@
##           develop     #903       +/-   ##
============================================
+ Coverage         0   47.84%   +47.84%     
============================================
  Files            0      650      +650     
  Lines            0    29121    +29121     
  Branches         0     7532     +7532     
============================================
+ Hits             0    13933    +13933     
- Misses           0    15130    +15130     
- Partials         0       58       +58     
Flag Coverage Ξ”
test 47.84% <ΓΈ> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
libs/ledger-live-common/src/api/Ethereum.ts 34.17% <0.00%> (ΓΈ)
...edgerjs/packages/hw-app-btc/src/getTrustedInput.ts 85.00% <0.00%> (ΓΈ)
libs/ledgerjs/packages/errors/src/index.ts 78.72% <0.00%> (ΓΈ)
libs/ledger-live-common/src/errors.ts 100.00% <0.00%> (ΓΈ)
libs/ledger-live-common/src/apps/runner.ts 54.90% <0.00%> (ΓΈ)
.../ledger-live-common/src/families/cardano/errors.ts 100.00% <0.00%> (ΓΈ)
...-common/src/families/bitcoin/js-synchronisation.ts 20.00% <0.00%> (ΓΈ)
...ger-live-common/src/countervalues/modules/index.ts 80.39% <0.00%> (ΓΈ)
...ledger-live-common/src/exchange/swap/checkQuote.ts 90.90% <0.00%> (ΓΈ)
...r-live-common/src/families/filecoin/transaction.ts 44.44% <0.00%> (ΓΈ)
... and 640 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

github-actions bot commented Aug 11, 2022

@juan-cortes

Screenshots: βœ…

There are no changes in the screenshots for this PR. If this is expected, you are good to go.

@gre
Copy link
Contributor

gre commented Aug 11, 2022

looks like it's not making LLD build :'(

@juan-cortes
Copy link
Contributor Author

CI not building caused by node-usb/node-usb#530
Working on a solution to bypass it in the meantime with @elbywan

@gre gre mentioned this pull request Aug 16, 2022
3 tasks
@github-actions github-actions bot added the desktop Has changes in LLD label Aug 16, 2022
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

There as been no activity on this PR for the last 14 days. Please consider closing this PR.

@github-actions github-actions bot added the Stale label Sep 1, 2022
@sagotch
Copy link

sagotch commented Sep 6, 2022

Hi. If I understood correctly, this PR should "only" needs testing? Are you interested in external testing (i.e. testing our app with this PR) or would I be wasting my time?

@gre
Copy link
Contributor

gre commented Sep 6, 2022

PR needs to continue:

@juan-cortes
Copy link
Contributor Author

Hi. If I understood correctly, this PR should "only" needs testing? Are you interested in external testing (i.e. testing our app with this PR) or would I be wasting my time?

Hi there! Although we didn't communicate on the conversation of this PR, it has been tested by our QA engineers and it's working. We need to rebase the branch on develop and point to a patch version of node-usb (if I got it right) and we should be good to go. Thanks for the offer though!

@juan-cortes
Copy link
Contributor Author

@gre This should be good to go now, dropped the patch, updated to 2.5.1 which doesnt have the yarn dep.

@gre gre merged commit 41d82e7 into develop Sep 8, 2022
@gre gre deleted the support/LIVE-3247 branch September 8, 2022 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants