Force commonjs imports for ledgerhq-react-native-hw-transport-ble #879
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
github-actions
bot
added
ledgerjs
| # 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 |
There was a problem hiding this comment.
interesting, i didn't even know about this file!
There was a problem hiding this comment.
yeah it's super useful to tweak pnpm behaviour
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| 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"; |
There was a problem hiding this comment.
Why are we using /lib here ?
There was a problem hiding this comment.
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.
Screenshots: ✅
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
📝 Description
Why?
Following this PR #364
@ledgerhq/deviceis 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
/libto 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
live-mobile✅ Checklist
📸 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 RNtransportBleTestcd RNtransportBleTest/npm i path/to/ledgerhq-react-native-hw-transport-ble-6.27.2.tgznpm install --save react-native-ble-plxcd ios && pod update && cd ..App.jsfile:npm start/npm iosShould print:
🚀 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.