Force commonjs imports for ledgerhq-react-native-hw-transport-ble #879

elbywan · 2022-08-09T09:04:32Z

📝 Description

Why?

Following this PR #364 @ledgerhq/device is now transpiled to commonjs and esm (in the /lib and /lib-es folders respectively).

Subpath exports have thus been added to use the same path (without /lib and /lib-es) in imports and allow consuming bundlers to declare which version of the dependency they want.

An unforeseen side-effect is that for the react-native package (ledgerhq-react-native-hw-transport-ble) consumers use exclusively the metro bundler which is incompatible with subpath exports.

Done

Adding /lib to the imports should work and should never cause duplicate issues since the metro bundler will always resolve the commonjs version of the package anyway.

Side effect:

📦 Updated pnpm to 7.9.0

❓ Context

Impacted projects: live-mobile
Linked resource(s): Unable to resolve module in react-native-hw-transport-ble #777

✅ Checklist

Test coverage
Atomic delivery
No breaking changes

📸 Demo

Steps to test:

from ledger-live

cd libs/ledgerjs/packages/react-native-hw-transport-ble/
pnpm pack (will generate ledgerhq-react-native-hw-transport-ble-6.27.2.tgz to the current folder)

from another folder

npx react-native init RNtransportBleTest
cd RNtransportBleTest/
npm i path/to/ledgerhq-react-native-hw-transport-ble-6.27.2.tgz
npm install --save react-native-ble-plx
cd ios && pod update && cd ..
Add this to the App.js file:

import TransportBLE from '@ledgerhq/react-native-hw-transport-ble';

console.log(TransportBLE);

npm start / npm ios

Should print:

 LOG  [Function BluetoothTransport]
 LOG  Running "RNtransportBleTest" with {"rootTag":1,"initialProps":{}}

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

vercel · 2022-08-09T09:04:34Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
live-common-tools	✅ Ready (Inspect)	Visit Preview	Aug 9, 2022 at 9:06AM (UTC)

3 Ignored Deployments

Name	Status	Updated
ledger-live-github-bot	⬜️ Ignored (Inspect)	Aug 9, 2022 at 9:06AM (UTC)
native-ui-storybook	⬜️ Ignored (Inspect)	Aug 9, 2022 at 9:06AM (UTC)
react-ui-storybook	⬜️ Ignored (Inspect)	Aug 9, 2022 at 9:06AM (UTC)

changeset-bot · 2022-08-09T09:04:35Z

⚠️ No Changeset found

Latest commit: 400a517

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

gre · 2022-08-09T09:20:48Z

.npmrc

-# See: https://github.com/actions/setup-node/issues/543#issuecomment-1182469390
-loglevel=warn
-# Disabled because it does not seem to be working and it regenerates the lockfile abusively
-side-effects-cache=false


interesting, i didn't even know about this file!

yeah it's super useful to tweak pnpm behaviour

https://pnpm.io/npmrc

codecov · 2022-08-09T09:26:11Z

Codecov Report

Merging #879 (400a517) into develop (d3d7c8d) will increase coverage by 3.68%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #879      +/-   ##
===========================================
+ Coverage    43.82%   47.51%   +3.68%     
===========================================
  Files          572      621      +49     
  Lines        24625    27873    +3248     
  Branches      6506     7174     +668     
===========================================
+ Hits         10793    13245    +2452     
- Misses       12742    13495     +753     
- Partials      1090     1133      +43

Flag	Coverage Δ
test	`47.51% <ø> (+3.68%)`	⬆️

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

Impacted Files	Coverage Δ
...s/packages/hw-app-eth/src/services/ledger/erc20.ts	`97.43% <0.00%> (ø)`
...s/hw-app-btc/src/shouldUseTrustedInputForSegwit.ts	`100.00% <0.00%> (ø)`
...js/packages/hw-app-btc/src/newops/psbtFinalizer.ts	`78.57% <0.00%> (ø)`
libs/ledgerjs/packages/hw-app-btc/src/constants.ts	`100.00% <0.00%> (ø)`
libs/ledgerjs/packages/hw-app-solana/src/Solana.ts	`100.00% <0.00%> (ø)`
.../ledgerjs/packages/hw-app-btc/src/newops/merkle.ts	`83.82% <0.00%> (ø)`
...-app-btc/src/startUntrustedHashTransactionInput.ts	`90.16% <0.00%> (ø)`
.../ledgerjs/packages/hw-app-btc/src/newops/policy.ts	`91.66% <0.00%> (ø)`
...bs/ledgerjs/packages/hw-app-btc/src/signMessage.ts	`86.84% <0.00%> (ø)`
...js/packages/hw-app-btc/src/newops/psbtExtractor.ts	`100.00% <0.00%> (ø)`
... and 39 more

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

Justkant · 2022-08-09T09:26:06Z

libs/ledgerjs/packages/react-native-hw-transport-ble/src/BleTransport.ts

-import { sendAPDU } from "@ledgerhq/devices/ble/sendAPDU";
-import { receiveAPDU } from "@ledgerhq/devices/ble/receiveAPDU";
+import { sendAPDU } from "@ledgerhq/devices/lib/ble/sendAPDU";
+import { receiveAPDU } from "@ledgerhq/devices/lib/ble/receiveAPDU";


Why are we using /lib here ?

This is the whole point of this PR 😉.

metro is unable to map @ledgerhq/devices/ble/… to either /lib/ble/… or /lib-es/ble/… because it does not support package exports.

Workarounds exist and are documented in the linked issue but it's not ideal at all to have to modify the metro config file.

Adding /lib here is not a big deal anyway since the react-native-hw-transport-ble package is only consumed by react-native clients and will never need to resolve the /lib-es version.

github-actions · 2022-08-09T09:31:31Z

@elbywan

Screenshots: ✅

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

elbywan added 2 commits August 9, 2022 10:50

chore: upgrade pnpm to 7.9.0

69fe12b

fix: force commonjs imports from react-native transport package

400a517

github-actions bot assigned elbywan Aug 9, 2022

github-actions bot added ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM labels Aug 9, 2022

elbywan linked an issue Aug 9, 2022 that may be closed by this pull request

Unable to resolve module in react-native-hw-transport-ble #777

Closed

gre approved these changes Aug 9, 2022

View reviewed changes

Justkant reviewed Aug 9, 2022

View reviewed changes

Justkant approved these changes Aug 9, 2022

View reviewed changes

elbywan merged commit d7d9251 into develop Aug 9, 2022

elbywan deleted the bugfix/native-transport-force-imports branch August 9, 2022 09:40

elbywan mentioned this pull request Aug 9, 2022

Unable to resolve module in react-native-hw-transport-ble #777

Closed

AlexandruPislariu pushed a commit to multiversx/ledger-live that referenced this pull request Aug 9, 2022

add changeset for LedgerHQ#879

3cc4543

elbywan mentioned this pull request Jan 12, 2023

Force commonjs imports from react-native transport package #2304

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force commonjs imports for ledgerhq-react-native-hw-transport-ble #879

Force commonjs imports for ledgerhq-react-native-hw-transport-ble #879

elbywan commented Aug 9, 2022 •

edited

Loading

vercel bot commented Aug 9, 2022 •

edited

Loading

changeset-bot bot commented Aug 9, 2022

gre Aug 9, 2022

elbywan Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

Justkant Aug 9, 2022

elbywan Aug 9, 2022 •

edited

Loading

github-actions bot commented Aug 9, 2022

Force commonjs imports for ledgerhq-react-native-hw-transport-ble #879

Force commonjs imports for ledgerhq-react-native-hw-transport-ble #879

Conversation

elbywan commented Aug 9, 2022 • edited Loading

📝 Description

Why?

Done

❓ Context

✅ Checklist

📸 Demo

Steps to test:

🚀 Expectations to reach

vercel bot commented Aug 9, 2022 • edited Loading

changeset-bot bot commented Aug 9, 2022

⚠️ No Changeset found

gre Aug 9, 2022

Choose a reason for hiding this comment

elbywan Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2022 • edited Loading

Codecov Report

Justkant Aug 9, 2022

Choose a reason for hiding this comment

elbywan Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2022

elbywan commented Aug 9, 2022 •

edited

Loading

vercel bot commented Aug 9, 2022 •

edited

Loading

elbywan Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

elbywan Aug 9, 2022 •

edited

Loading