feat: upgrade bn.js from 4.12 to 5.2 #1121
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/blockstack/stacks-js/3xmSah7pty8JFaaQPMjbdgVVN83r |
Codecov Report
@@ Coverage Diff @@
## master #1121 +/- ##
=======================================
Coverage 62.64% 62.64%
=======================================
Files 113 113
Lines 8196 8196
Branches 1529 1529
=======================================
Hits 5134 5134
Misses 2947 2947
Partials 115 115 Continue to review full report at Codecov.
|
| @@ -31,11 +31,11 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@stacks/common": "^2.0.1", | |||
| "@types/bn.js": "^4.11.6", | |||
| "@types/bn.js": "^5.1.0", | |||
| "@types/node": "^14.14.43", | |||
| "bip39": "^3.0.2", | |||
| "bitcoinjs-lib": "^5.2.0", | |||
There was a problem hiding this comment.
IIRC it was the bitcoinjs family of libs (this one and some of the bip libs) that were including bn.js v4. Have you tested that these dependencies don't end up requiring bn.js v4 to be bundled anyway?
There was a problem hiding this comment.
Other packages are depending of v4, my idea was to do the upgrade gradually, here is the list of deps still using it (might have missed some):
- @stacks/keychain
- @blockstack/rpc-client 0.3.0-alpha.11
- @blockstack/stacks-transactions 0.5.1
- @blockstack/rpc-client 0.3.0-alpha.11
- Used by multiple packages
- jsontokens 3.0.0
- asn1.js 5.4.1
- jsontokens 3.0.0
- @stacks/cli 2.0.1
- blockstack 19.3.0
- Used by multiple packages
- elliptic 6.4.5
- bitcoin-lib 5.2.0
- tiny-secp256k1 1.1.6
The issue can be that some of them seem unmaintained, like indutny/asn1.js#118 where the pr was opened almost 2 years ago.
There was a problem hiding this comment.
I'd be more concerned with getting the elliptic package and bitcoinjs-lib packages upgraded to bn.js v5. Is there anything in bn.js v5 that is needed? And would be more feasible to change projects using v5 to use v4 instead?
There was a problem hiding this comment.
Looks like bitcoin-js lib is working on removing the dependency to bn.js as they are dropping tiny-secp256k1 bitcoinjs/bitcoinjs-lib#1734. For elliptic it looks more challenging.
I have no idea if you are using v5 specific features on the other packages.
There was a problem hiding this comment.
We are only dropping bn.js from the main package, ecpair and bip32 will still use it until we can upgrade them to use WASM.
|
Hi @pradel -- based on the comments, it doesn't look like this PR would actually help reduce the bundle size. I'm going to close it for now, but please reopen if you disagree. Meanwhile, we are looking into all the different pain points with stacks.js, including bundle sizes, and hope to have a plan we can share with the community in the coming weeks. Thanks! |
Description
At the moment @stacks/connect is using bn.js version 5.2 and the packages of this repo the version 4.12. This means that when we use both connect and stacks.js packages in our app, we will end up having 2 versions of bn.js in our bundle.
What I did was to upgrade bn.js dependency from 4.12 to the next major 5.2 so both connect and stacks.js use the same version.
stacks-wallet-web is already using 5.2 too https://github.com/blockstack/stacks-wallet-web/blob/main/package.json#L71
Related to issue #1120, I noticed that bn.js is included twice in my bundle.
Type of Change
Does this introduce a breaking change?
All the tests are passing so not sure if this qualifies as a breaking change as users won't be impacted directly by this change.
Are documentation updates required?
I don't think so.
Checklist
npm run testpasses