Standardize bn.js dep across monorepo #3262

cgewecke · 2019-12-08T08:40:44Z

Description

NB: This PR targets the buildsize checker PR #3260, prerequisite for merging here

(Tries to) reduce the bundle size to provide a baseline measurement for how small the gulp/browserify build could get.

Internalizes the two silentcicero dependencies in web3-utils so that BN.js can be standardized to 4.11.8 across the entire project. The deps seem abandoned.
- commit cce191f
- build size: 1.14 MB :/
- improvement: none (See build)

Type of change

Dependencies

Checklist:

coveralls · 2019-12-08T19:54:06Z

Coverage increased (+0.05%) to 84.854% when pulling aeb1f46 on issue/1178 into f0f9ffc on ci/add-buildsize-checker.

cgewecke · 2019-12-09T06:29:40Z

@nivida My question for the initial review is:

Do you think this is worth doing? e.g Could remove the shakefiy and get CI passing. The bn.js would be deduped. But there'd be no measurable build size improvement - is it still a good idea?

It might help people using webpack.

nivida · 2019-12-09T08:31:32Z

Do you think this is worth doing? e.g Could remove the shakefiy and get CI passing. The bn.js would be deduped. But there'd be no measurable build size improvement - is it still a good idea?

I've achieved the same with adding of custom resolver rules (the dedupe parameter of the rollup config function). As the build size reduction does show are there still some duplications of bn.js around (bn.js ~80kb; webpack bundle analyzer should show them). Complete deduplication of js-sha3, bn.js, and elliptic was bringing up the bundle size improvement. I think if we use shakeify and update the cjs imports to a common shakable way in all packages will it probably improve the situation as well.

I think we better keep the code as it is and try to add custom resolver rules to the browserify settings to dedupe the bn.js, js-sha3, and elliptic dependency. By side of the bundle size improvement is it probably anyways better to manage the included version of those listed dependencies on our own (security risky dependencies).

cgewecke · 2019-12-09T10:03:36Z

@nivida

I think we better keep the code as it is and try to add custom resolver rules to the browserify settings to dedupe the bn.js, js-sha3, and elliptic dependency. By side of the bundle size improvement is it probably anyways better to manage the included version of those listed dependencies on our own (security risky dependencies).

For clarification, you think these changes should stay in and maybe I should remove shakeify from this PR and the optimizations should go in separately?

nivida · 2019-12-09T10:16:58Z

@cgewecke

For clarification, you think these changes should stay in and maybe I should remove shakeify from this PR and the optimizations should go in separately?

I think it will be possible to achieve the same or even more with custom resolver rules instead of using the shakeify package and copying of dependencies over. ~~Such resolver rules are also way more future proof than going through the deps and copying them over if required or checking for duplications on the addition of a dependency.~~

alcuadrado · 2019-12-10T15:01:55Z

Can you elaborate on why custom resolution rules are needed? I've never seen that case before, so I can't follow it. Is there a shared limitation by browserify and rollup that makes them needed with both tools?

nivida · 2019-12-10T15:45:04Z

Can you elaborate on why custom resolution rules are needed? I've never seen that case before, so I can't follow it.

It is to deduplicate those dependencies over the whole lib which does, in the end, lower the minified bundle size.

Is there a shared limitation by browserify and rollup that makes them needed with both tools?

Rollup and also Browserify does not remove all duplicated dependencies as it should during tree shaking and that’s why those resolver rules are required. The used bn.js versions are all on the same minor version which means Rollup and Browserify should both be able to de-duplicate it ~~but sadly they can't handle it correctly.~~

alcuadrado · 2019-12-10T16:04:40Z

Rollup and also Browserify does not remove all duplicated dependencies as it should during tree shaking and that’s why those resolver rules are required. The used bn.js versions are all on the same minor version which means Rollup and Browserify should both be able to de-duplicate it but sadly they can't handle it correctly.

I'm inclined to think that the fault is on our side and that it's just a misconfiguration or a poor dependencies choice. I'd need a link to their docs explaining this limitation, or to a bug report describing this behavior, to be convinced. The reason is that otherwise, custom resolution rules would be super common, and they aren't.

I'll open an issue to discuss this, as properly solving this will probably need lots of planning and discussing.

nivida · 2019-12-10T16:50:37Z

@alcuadrado

or a poor dependencies choice

Yes, this is the case and the reason why @cgewecke did copy the ethjs and number-to-bn dependency over to web3.js. We can keep those copied dependencies, we use the utility methods from ethers, or we define global resolver rules for our minified bundle. But I think the most future proof and also the most stable way would be to use the ethers utilities because those are small, do use bn.js, and are managed by Richard from the EF-JS-Team. With further dependency clean up PRs is it probably also possible to achieve the bundle size from PR #3160.

I'd need a link to their docs explaining this limitation

If I do remember correctly was rollup able to de-duplicate all bn.js dependencies which are in our package.json files but it wasn't able to de-duplicate it if a sub-package dependency has a bn.js dependency listed in their package.json. webpack does have the same problem as rollup and this was the reason why my starter test env has some resolver rules defined.

Edit:
I've noticed the de-duplication issue we have is because some dependencies have bn.js and other dependencies defined as fixed version and it can't de-duplicate them also if only the patch level differs.

alcuadrado · 2019-12-11T18:49:20Z

Update on the bn.js deduplication:

I further analyzed the dependencies tree while preparing #3267. Rollup and browserify behavior (i.e. not deduplicating bn.js) is correct. Forcing it to deduplicate is a huge risk, as nothing guarantees that it will work.

There are dependencies that require incompatible versions of bn.js, and that should be respected by any bundler, and this is done by bundling two or more versions.

For example, web3-utils require the exact version 4.11.8, and also number-to-bn which in turn requires 4.11.6.

I think the path followed by @cgewecke in this PR is a movement in the right direction.

Note that not being able to deduplicate dependencies is what I've been warning about for some time, for example in #3186. Because of pinning dependencies, the situation is already complicated for us and is at least as complicated, if not worse, for our users.

alcuadrado · 2019-12-11T18:50:04Z

packages/web3-utils/src/ethjsUnit.js

+Note, Richard is a god of ether gods. Follow and respect him, and use Ethers.io!
+*/
+
+const BN = require('bn.js');


Isn't ethers a dependency? Why is this duplicated?

That's a good question...discussed a bit below. This code is a few years old now and swapping it for ethers will take a little additional research.

alcuadrado · 2019-12-11T18:51:01Z

packages/web3-utils/package.json

        "randombytes": "^2.1.0",
+        "strip-hex-prefix": "1.0.0",


This is probably causing a dependency duplication in another packages, as IIRC strip-hex-prefix is a dependency of ethereumjs-util, which is inderectly used by web3.

Looked into further - the weight of this package (which uses it's own tiny dependency, is-hex-prefix) is < 20 lines, un-minified. It has a single published version from 3 years ago and is maintained by silentcicero.

It looks pinned to 1.0.0 all the way through the tree:

ethjs-util pins both deps here.

ethereumjs-utils pins ethjs-util here

This stuff is small and could easily be internalized somewhere although the benefit might be hard to measure.

SilentCicero continues to actively develop eth stuff but is working on optimistic rollup sidechains...

That's everything I found out :) and am persuadable on changing this, what's your assessment?

[EDITED] On a fresh install of ethereumjs-util in an empty package the npm list bn.js is:

└─┬ ethereumjs-util@6.2.0 ├── bn.js@4.11.8 ├─┬ rlp@2.2.4 │ └── bn.js@4.11.8 deduped └─┬ secp256k1@3.7.1 ├── bn.js@4.11.8 deduped └─┬ elliptic@6.5.2 └── bn.js@4.11.8 deduped

It looks like we should get deduped...

alcuadrado · 2019-12-11T18:52:12Z

packages/web3-utils/src/numberToBN.js

+ * @return {Object} `output` BN object of the number
+ * @throws if the argument is not an array, object that isn't a bignumber, not a string number or number
+ */
+module.exports = function numberToBN(arg) {


Is this function exposed in web3's API? If not, I'd remove it. It's a a huge mess.

This will be used internally for some utility functions but could probably get replaced with the ethers BigNumber wrapper object.

It internal to toBN and fromWei both of which are heavily used.

Personally I'd be more comfortable removing this in a different PR that rationalized use of Ethers utils and where we have tests in place to make sure we don't accidentally introduce unexpected behavior. Also where we double-check the Ethers issues about differences people see in how Ethers utils behave vs. Web3 since the code here is 3 years old.

PR bumps BN across some patch versions - might be worth isolating.

nivida · 2019-12-12T08:22:38Z

Forcing it to deduplicate is a huge risk, as nothing guarantees that it will work.

Because only the patch level does differ did I thought is there no risk to apply such custom resolver rules but thanks for the closer check!

I think the path followed by @cgewecke in this PR is a movement in the right direction.

Yes, it is definitely the right direction but I would recommend replacing those dependencies with equivalent utility functions from ethers. This would be aligned with the overall EF-JS-Team goal and would also improve the situation for us here :)

cgewecke · 2019-12-12T19:26:37Z

I think the path followed by @cgewecke in this PR is a movement in the right direction.

Is this still right? The scope of the PR is to reduce the bundle size a bit but there's no measurable benefit here.

In turn, looking over this stuff makes it clear that we should depend more on the Ethers implementations but doing that might require more care (see comment).

@nividia are you confident this change will pay dividends if the bundling is fixed?

nivida · 2019-12-18T09:07:55Z

Is this still right? The scope of the PR is to reduce the bundle size a bit but there's no measurable benefit here.

The scope of this PR didn't change in my point of view it is still flatting of the dependency tree and improving the bundle size. This because flatting/deduping of the dependency tree should improve the bundle size of bundles created with parcel or webpack or we do not actually dedupe/flatten it.

@nividia are you confident this change will pay dividends if the bundling is fixed?

If no measurable benefit is existing will this also not improve anything with the fixed build pipeline.

Edit:

or we do not actually dedupe/flatten it.

It could also be the case that the bundler is deduplicating it (parcel is probably doing this) and we do not notice any change.

nivida · 2020-01-17T14:21:40Z

@cgewecke I think we should held a call related to the bundle size improvements for splitting this up in smaller tasks with clear goals etc. WDYT?

cgewecke · 2020-01-20T06:39:18Z

@nivida Yes definitely.

nivida · 2020-01-21T09:24:08Z

Closed because we will plan/discuss the bundle size improvements first together.

cgewecke force-pushed the issue/1178 branch from b5b30d2 to cce191f Compare December 8, 2019 19:45

cgewecke force-pushed the issue/1178 branch from c6f902f to cce191f Compare December 8, 2019 20:00

cgewecke requested a review from nivida December 8, 2019 21:31

cgewecke mentioned this pull request Dec 8, 2019

Build is smaller on second install? #3263

Closed

cgewecke changed the title ~~Reduce build size~~ Standardize bn.js dep across monorepo Dec 9, 2019

alcuadrado mentioned this pull request Dec 10, 2019

A possible strategy to reduce bundle sizes #3267

Closed

cgewecke marked this pull request as ready for review December 10, 2019 20:34

cgewecke added 1.x 1.0 related issues dependencies Review Needed Maintainer(s) need to review labels Dec 10, 2019

cgewecke added 2 commits December 10, 2019 14:35

Internalize number-to-bn, ethjs-unit to normalize bn.js dep

1586c59

Add common-shakeify to gulp browserify step

9463edb

cgewecke force-pushed the issue/1178 branch from ea1bd73 to aeb1f46 Compare December 10, 2019 22:37

Remove shakeify

aeb1f46

alcuadrado reviewed Dec 11, 2019

View reviewed changes

nivida changed the base branch from ci/add-buildsize-checker to 1.x December 12, 2019 08:34

nivida closed this Jan 21, 2020

nivida deleted the issue/1178 branch February 3, 2020 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize bn.js dep across monorepo #3262

Standardize bn.js dep across monorepo #3262

cgewecke commented Dec 8, 2019 •

edited by nivida

Loading

coveralls commented Dec 8, 2019 •

edited

Loading

cgewecke commented Dec 9, 2019 •

edited

Loading

nivida commented Dec 9, 2019

cgewecke commented Dec 9, 2019 •

edited

Loading

nivida commented Dec 9, 2019 •

edited

Loading

alcuadrado commented Dec 10, 2019

nivida commented Dec 10, 2019 •

edited

Loading

alcuadrado commented Dec 10, 2019

nivida commented Dec 10, 2019 •

edited

Loading

alcuadrado commented Dec 11, 2019

alcuadrado Dec 11, 2019

cgewecke Dec 12, 2019

alcuadrado Dec 11, 2019

cgewecke Dec 12, 2019 •

edited

Loading

cgewecke Dec 12, 2019 •

edited

Loading

alcuadrado Dec 11, 2019

nivida Dec 12, 2019

cgewecke Dec 12, 2019

nivida commented Dec 12, 2019

cgewecke commented Dec 12, 2019 •

edited

Loading

nivida commented Dec 18, 2019 •

edited

Loading

nivida commented Jan 17, 2020

cgewecke commented Jan 20, 2020

nivida commented Jan 21, 2020

Standardize bn.js dep across monorepo #3262

Standardize bn.js dep across monorepo #3262

Conversation

cgewecke commented Dec 8, 2019 • edited by nivida Loading

Description

Type of change

Checklist:

coveralls commented Dec 8, 2019 • edited Loading

cgewecke commented Dec 9, 2019 • edited Loading

nivida commented Dec 9, 2019

cgewecke commented Dec 9, 2019 • edited Loading

nivida commented Dec 9, 2019 • edited Loading

alcuadrado commented Dec 10, 2019

nivida commented Dec 10, 2019 • edited Loading

alcuadrado commented Dec 10, 2019

nivida commented Dec 10, 2019 • edited Loading

alcuadrado commented Dec 11, 2019

alcuadrado Dec 11, 2019

Choose a reason for hiding this comment

cgewecke Dec 12, 2019

Choose a reason for hiding this comment

alcuadrado Dec 11, 2019

Choose a reason for hiding this comment

cgewecke Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

cgewecke Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

alcuadrado Dec 11, 2019

Choose a reason for hiding this comment

nivida Dec 12, 2019

Choose a reason for hiding this comment

cgewecke Dec 12, 2019

Choose a reason for hiding this comment

nivida commented Dec 12, 2019

cgewecke commented Dec 12, 2019 • edited Loading

nivida commented Dec 18, 2019 • edited Loading

nivida commented Jan 17, 2020

cgewecke commented Jan 20, 2020

nivida commented Jan 21, 2020

cgewecke commented Dec 8, 2019 •

edited by nivida

Loading

coveralls commented Dec 8, 2019 •

edited

Loading

cgewecke commented Dec 9, 2019 •

edited

Loading

cgewecke commented Dec 9, 2019 •

edited

Loading

nivida commented Dec 9, 2019 •

edited

Loading

nivida commented Dec 10, 2019 •

edited

Loading

nivida commented Dec 10, 2019 •

edited

Loading

cgewecke Dec 12, 2019 •

edited

Loading

cgewecke Dec 12, 2019 •

edited

Loading

cgewecke commented Dec 12, 2019 •

edited

Loading

nivida commented Dec 18, 2019 •

edited

Loading