New build pipeline

[nivida]

Fixes #3155 #3074 #2623 #1178

Description

Because of repeating issues (#3155) we had with the minified bundle did I back-port and extend the build pipeline of the 2.x branch.

Key Facts:

• Rollup with the related plugins will be used to bundle, transform and minify the web3 bundles.
• Karma setup updated (it wasn't using the minified file from the project)
• Bundle sizes optimized
  • parcel zero-config bundle: 1.12 MB -> 1.04 MB
  • web3.min.js: 1.1MB -> 762KB
• Modern JS can now be used anywhere
• CJS, ESM, and minified UMD bundles are now existing for each web3 package
• Babel transforms the bundle for the minified file to support the last 2 browser versions
• Babel transforms the CJS and ESM bundle to support nodejs v8+
• eventemitter3 dependency updated in PromiEvent package
  • I've checked the release announcements for breaking changes
• ENS.js moved to index.js for consistency reasons

Scripts

• build:web3 - Creates just the CJS and ESM bundle of the web3 umbrella package
• build:web3:minified - Creates all CJS and ESM bundles of all packages and creates a web3.min after
• build:all - Creates CJS and ESM bundles for all packages
• build:all:cjs - Creates CJS bundles for all packages
• build:all:esm - Creates ESM bundles for all packages
• build:all:minified - Creates minified UMD bundles with the cjs bundles as the source for all packages
• build:all:release - Creates CJS, ESM, and minified UMD bundles for all packages without instrumenting the code for the istanbul code coverage report.

New Package Entries

• main
  • Will be used in nodejs on a normal require(...) and does have the CJS module format.
• module
  • Will be used for ESM-ready tools and does have the ES format.
• unpkg
  • Will be used from the unpkg CDN and does contain the minified UMD formatted bundle.
• jsdelivr
  • Will be used from the jsdelivr CDN and does contain the minified UMD formatted bundle.

Rollup Config Setup

The base configuration is located in the root folder of this repository and does return the configuration function which will be used in each package of this project.

Properties of the config function:

• name: string
  • Will be used for named exports
• outputFileName: string
  • Name of the output file
• globals?: {[key: string]: string}
  • pre-defined names for the globally available packages (used in CJS and ESM)
• dedupe?: string[]
  • Will be used to remove duplicated modules in the minified UMD bundles.
• namedExports?: boolean``
  • Simple config to activate named exports for the bundles of a package

Example Usage:

import pkg from './package.json';
import rollupConfig from '../../rollup.config';

export default rollupConfig(
    'Web3Net',
    pkg.name,
    {
        'web3-core': 'Web3Core',
        'web3-core-method': 'Web3CoreMethod',
        'web3-utils': 'Web3Utils'
    },
    ['bn.js', 'elliptic', 'js-sha3', 'underscore']
);

[cgewecke]

@nivida Apologies, I know you're still working on this... was just peeking at it and left a couple initial notes.

Because these are really big changes to how the project is built & published, it might be a good idea to merge #3157 (after review) and check them against it.

[nivida]

@alcuadrado

It's really hard to review a PR this big, so we can be missing things.

I think our tests do help us to have the proof that we aren't missing something. The existing unit tests and the newly added e2e tests do give us enough proof. Additionally were three devs (me included) reviewing this PR.

Are we sure we are testing the browser builds correctly? The browser tests are browserified before run, which can be masking problems that would only be observable with e.g. parcel.

Browserify, Parcel, or Webpack all of those are bundlers and all of those do add some polyfills if required or not. We will always have the possibility that one of those (whatever bundler we use for our browser tests) does add something we didn't expect he does. A simple test case which does only include the minified bundle and initiate the Web3 object without any pre-processor added etc. would probably give us a better proof if all deps are bundled correctly.

I'm not convinced that the dependencies are handled properly. There are some weird things going. I left some comments about this.

Dependencies have to be excluded in normal CJS, ESM, or UMD bundles. Only the minified browser bundles do include them. This gives our developers the freedom to manage the dependencies versions with adding of custom resolver rules etc. in their build pipeline.

I'm not convinced that polyfills are correctly handled either. If only the last two versions of every major browser is used, why do we even polyfill some things?

Because of node version 8 !== features last 2 browser versions.

Are we sure that the ESM modules are actually tree-shakeable? Is this tested? Is this event testable? We shouldn't release the ESM modules if we aren't 100% sure, as once people start depending on them, changing how we export them would probably be a breaking change.

Yes, I've tested it. If it wouldn't be tree-shakeable would it also not be possible to use this lib at all. This because the added ESM imports/exports are the only reason why it can get tree-shaked. But there is definitely still enough we could improve later.

Are we sure that we don't need a browser field anymore? Again, this has only been tested with browserify.

Yes, I am.

The rollup config files are too complex. It's really hard to get the whole picture of what's going on in each build. IMO being explicit and readability are much more important than avoiding repetition for config files.

I don't think it is that complex. 3 module structures, 2 rollup plugin setups, 2 babel configs.

Anyway, this is a huge change, and it's looking good. I just wouldn't hurry to merge it and release it, as it introduces a lot of instability to the project, and has the potential of accidentally break things.

I don't think this introduces any kind of instability or is there a specific example from you? Why are you making that assumption up that this will break stuff?

[nivida]

@alcuadrado
Btw.: It would be great if the defined source map URLs are correct for all ethereumjs packages:

[nivida]

These are the supported browsers with defining of last 2 versions as the target for the minified UMD bundles (browser bundles):

and_chr 76
and_ff 68
and_qq 1.2
and_uc 12.12
android 76
baidu 7.12
bb 10
bb 7
chrome 77
chrome 76
edge 18
edge 17
firefox 69
firefox 68
ie 11
ie 10
ie_mob 11
ie_mob 10
ios_saf 13.0-13.1
ios_saf 12.2-12.3
kaios 2.5
op_mini all
op_mob 46
op_mob 12.1
opera 62
opera 60
safari 13
safari 12.1
samsung 10.1
samsung 9.2

[alcuadrado]

There are multiple interconnected things going on in the comments, and some of them are hard to answer without paying attention to the bigger picture, so I'm answering many of them here.

Supported browsers and babel config

last 2 versions does mean more as you thought

You are right. I though you were talking about the last two versions of the major browsers, not the last 2 versions browserlist query. This brought some misunderstanding.

I reviewed the list of browsers that you posted (thanks for that!), and something caught my attention immediately: there are some unsupported (by their makers) browser with 0% market-share there.

I started googling about it, and found this babel issue, and this site by one of the babel maintainers which explains why you shouldn't use that. The main thing is that last 2 versions includes every abandoned browser, so you are stuck with recompiling everything forever.

Browserlist config

That article I linked is great, but somewhat old. Now browserlist has a recommended default query (defaults), which we should probable use. Maybe also adding info about node. Something like defaults, maintained node versions.

@babel/runtime

If we use defaults, or the config recommended in the site I linked, we get to support fewer browsers, but we still have to compile async functions. This resolves the @babel/runtime comments, as it is needed. We should use it, but only bundle them once in the minified files. I think we don't need to do anything in particular for this to happen. Am I right?

Syntax proposal plugins

While I think they are not worth it, I don't care much about them. I just wanted to raise the point that introducing things like this has to be addressed individually and discussed in the context of the project objectives. Adding this kind of things to a PR like this one prevents that discussion from happening, as no-one wants to block progress over a minor thing like that. I'll mark those comments as resolved because of that.

[alcuadrado]

Are we sure we are testing the browser builds correctly? The browser tests are browserified before run, which can be masking problems that would only be observable with e.g. parcel.

A simple test case which does only include the minified bundle and initiate the Web3 object without any pre-processor added etc. would probably give us a better proof if all deps are bundled correctly.

This was the kind of thing I had in mind, yes.

The ideal test suit would also run all the tests using the UMD build without any bundle.

I'm not convinced that the dependencies are handled properly. There are some weird things going. I left some comments about this.

Dependencies have to be excluded in normal CJS, ESM, or UMD bundles. Only the minified browser bundles do include them. This gives our developers the freedom to manage the dependencies versions with adding of custom resolver rules etc. in their build pipeline.

I understand that that's the intention. But I don't follow what you did with the devDependencies. I'm not saying that it's wrong either. It's just uncommon and undocumented.

I'm not convinced that polyfills are correctly handled either. If only the last two versions of every major browser is used, why do we even polyfill some things?

Because of node version 8 !== features last 2 browser versions.

This was addressed in my previous comment.

Are we sure that the ESM modules are actually tree-shakeable? Is this tested? Is this event testable? We shouldn't release the ESM modules if we aren't 100% sure, as once people start depending on them, changing how we export them would probably be a breaking change.

Yes, I've tested it. If it wouldn't be tree-shakeable would it also not be possible to use this lib at all. This because the added ESM imports/exports are the only reason why it can get tree-shaked. But there is definitely still enough we could improve later.

I'm glad you tested it, but I had to ask, as you didn't mentioned it. I think the PR description is a great place to do it.

Are we sure that we don't need a browser field anymore? Again, this has only been tested with browserify.

Yes, I am.

I was mostly asking about the reason it can be removed, not trying to challenge you. Again, maybe the PR description would have been the right place to document this.

It's still unclear to me why though. Can you clarify it?

The rollup config files are too complex. It's really hard to get the whole picture of what's going on in each build. IMO being explicit and readability are much more important than avoiding repetition for config files.

I don't think it is that complex. 3 module structures, 2 rollup plugin setups, 2 babel configs.

Sorry, complex wasn't the right word. The configs itself don't seem complex, they are just very very hard to read. I don't have the complete picture in mind yet, just part of it, and I spent a few hours on them.

Anyway, this is a huge change, and it's looking good. I just wouldn't hurry to merge it and release it, as it introduces a lot of instability to the project, and has the potential of accidentally break things.

I don't think this introduces any kind of instability or is there a specific example from you? Why are you making that assumption up that this will break stuff?

Every change introduces some instability, as there's always a possibility of breaking things. This doesn't mean that all of them break things though. But the bigger the PR, the more changes it makes, the higher the possibility of any of those changes going wrong.

I don't assume that this is breaking anything though, otherwise I would be really explicit about it. I'm just trying to understand that this PR wrings, and in the meantime, being extra cautious.

[alcuadrado]

Btw.: It would be great if the defined source map URLs are correct for all ethereumjs packages:

I created this issue ethereumjs/ethereumjs-config#21 to discuss this. Would you mind mention it there that you also think sources should be included, @nivida ?

[cgewecke]

@nivida
You've already identified this as a something to add in #3192- just copying over your review note

We can speed up the mentioned development process with adding of the build commands to all web3 packages to be able to watch or manually build a single package.

---

A simple test case which does only include the minified bundle and initiate the Web3 object without any pre-processor added etc. would probably give us a better proof if all deps are bundled correctly.

Yes - this would be a nice sanity check. But I think we still have to run tests across the body of the code since the build can be defective in a way that doesn't error until you try to execute something specific - #3155 is an example. The existing browserify setup (on 1.x) does detect that the 1.2.2 bundle is corrupted. If we remove the babel/polyfill added in Karma, the tests error as they should. I misdiagnosed this a meta problem because I thought the underlying bug had been fixed in #3062.

I agree with @alcuadrado that this is moving is really good direction, (it will be v. useful for helping with the build size) but also think that we should exercise caution with it and have a chance to use it a bit / think about it before publishing with it.

@alcuadrado Thanks for such a thorough review! Great. 💯
@nivida This is a really substantial block of work, very important. 🏅

[nivida]

@alcuadrado

I started googling about it, and found this babel issue, and this site by one of the babel maintainers which explains why you shouldn't use that. The main thing is that last 2 versions includes every abandoned browser, so you are stuck with recompiling everything forever.

Great! Thanks for checking this closer!

Browserlist itself does use the following query for browsers last 1 version, > 1%, not dead and for node does it use maintained node versions.

Node versions:

node 8.16.0
node 13.0.0
node 12.13.0
node 10.17.0

Browsers and versions:

and_chr 76
and_ff 68
and_qq 1.2
and_uc 12.12
android 76
baidu 7.12
chrome 77
chrome 76
edge 18
firefox 69
ie 11
ie_mob 11
ios_saf 13.0-13.1
ios_saf 12.2-12.3
kaios 2.5
op_mini all
op_mob 46
opera 62
safari 13
safari 12.1
samsung 10.1
samsung 9.2

That article I linked is great, but somewhat old. Now browserlist has a recommended default query (defaults), which we should probable use. Maybe also adding info about node. Something like defaults, maintained node versions.

The defaults set:

and_chr 78
and_ff 68
and_qq 1.2
and_uc 12.12
android 76
baidu 7.12
chrome 78
chrome 77
chrome 76
chrome 49
edge 18
edge 17
firefox 70
firefox 69
firefox 68
ie 11
ie_mob 11
ios_saf 13.0-13.2
ios_saf 12.2-12.4
kaios 2.5
op_mini all
op_mob 46
opera 64
opera 63
safari 13
safari 12.1
safari 5.1
samsung 10.1
samsung 9.2

I would probably recommend to not use the defaults setting as well because of the chrome 49 browser who is existing in this list. The set Browserlist is using is more restrictive but does include in my view of point all actually used Browsers and their versions.

If we use defaults, or the config recommended in the site I linked, we get to support fewer browsers, but we still have to compile async functions. This resolves the @babel/runtime comments, as it is needed. We should use it, but only bundle them once in the minified files. I think we don't need to do anything in particular for this to happen. Am I right?

Yep, you are. Quote from the Babel documentation:

This is where the @babel/plugin-transform-runtime plugin comes in: all of the helpers will reference the module @babel/runtime to avoid duplication across your compiled output. The runtime will be compiled into your build.

While I think they are not worth it, I don't care much about them. I just wanted to raise the point that introducing things like this has to be addressed individually and discussed in the context of the project objectives. Adding this kind of things to a PR like this one prevents that discussion from happening, as no-one wants to block progress over a minor thing like that. I'll mark those comments as resolved because of that.

Great thanks! I will next time update the PR description for a second review to explain such changes closer instead of waiting until the reviewer is asking.

I understand that that's the intention. But I don't follow what you did with the devDependencies. I'm not saying that it's wrong either. It's just uncommon and undocumented.

I think you quoted the wrong comment from our discussion. But yes we were misunderstanding us. I was talking about the normal dependencies and that those only have to be included in the minified browser bundles. Just for those who come by: The devDependencies do exist to smaller the bundle size of the minified browser bundle with custom deduplicating resolver rules (also mentioned in the PR description in the section "Rollup Config Setup").

I was mostly asking about the reason it can be removed, not trying to challenge you. Again, maybe the PR description would have been the right place to document this.

Yes, I've proposed this PR a while ago and created a correct PR description. After the first review and some smaller discussions with @cgewecke did we sadly change the base setup and I didn't update the PR description after. Sorry for this confusion.

It's still unclear to me why though. Can you clarify it?

The browser entry will be used by all bundlers and does have a higher priority than the module entry who would have ES modules. We could add the browser entry again and define the ESM bundles for this property but it wouldn't make any difference to the current setup.

Sorry, complex wasn't the right word. The configs itself don't seem complex, they are just very very hard to read. I don't have the complete picture in mind yet, just part of it, and I spent a few hours on them.

Okay, you struggled with the code complexity and not with the configuration complexity itself. Yes, the config "generator" can be written in a cleaner more readable way. I will improve the readability of the config "generator" asap. :)

Every change introduces some instability, as there's always a possibility of breaking things. This doesn't mean that all of them break things though. But the bigger the PR, the more changes it makes, the higher the possibility of any of those changes going wrong.

I see. It was thought of as a cautionary reminder. 👌

I created this issue ethereumjs/ethereumjs-config#21 to discuss this. Would you mind mention it there that you also think sources should be included, @nivida ?

I think the source map in the published NPM package should point on the dist file instead of the actual sources. But yes, I will drop a comment there and probably also do it as the first ethereumjs task 👌

@cgewecke

You've already identified this as a something to add in #3192- just copying over your review note

Those capabilities are already implemented and a global watch script which doesn't bring your PC on fire got added as well ;)

agree with @alcuadrado that this is moving is really good direction, (it will be v. useful for helping with the build size) but also think that we should exercise caution with it and have a chance to use it a bit / think about it before publishing with it.

Yep, same here. I will check your hotfix PRs and release them later this evening (CEST) or tomorrow morning. I think this PR is the first step of the cleanup and brings a modern build pipeline to this project. Can't wait to improve it more after ;)

[nivida]

@cgewecke @alcuadrado I've updated the targeted browsers in the babel config, checked the dedupe rules after the module structure change, and removed the rollup devDep from all packages. I will keep the current structure of the rollup config "generator". We can improve this later more when we restructured the project to a simpler folder/package structure.

Edit:
I've removed the native crypto dependency because of potential problems it could have with the polyfills rollup-plugin-node-builtins does provide. The crypto dependency in web3-eth-accounts got replaced with the correct modules from crypto-browserify. Those newly added modules are already in-usage in 2.x/1.x and were before imported by the crypto-browserify package. The pbkdf2, browserify-cipher, and randomBytes will use the native crypto package if existing.

[cgewecke]

@nivida and I discussed this in a meeting on Nov 25.

The main purpose of this PR is to reduce the build-size to address #1178. The main blocker is reviewer fear of its size & scope.

Producing cjs & umd modules is the source of much of the complexity here. There are size-reduction benefits to these. Unfortunately it turns out tree-shaking didn't make the bundle that much smaller - @nivida estimated it was a few percent.

TLDR; one way forward is to use all the work here as a source but open a new PR (or PRs) with a more modest goal...

1. Add a build-size checking service to CI, so we can begin monitoring this in every PR.
2. Add a (failing/skipped) test for the current min bug which has a broken browser entry point
3. Remove all duplicate deps.
4. Add rollup but only generate the min.

Proceeding this way means we can

• avoid making changes to the 'real' codebase
• keep the existing dev environment simple.
• upgrade the build tool (and publish more advanced products in future as necessary.)
• address Compiled web3 bundle is HUGE #1178.
• fix the build using a rollup config that is known to work on 2.x

[nivida]

I've closed this PR because the changes here were too big but the branch will still exist. We will slowly apply the improvements from this branch to the current build pipeline as for example with PR #3261.

[ghost]

@nivida how is this coming up? I'm particularly interested in the esm import feature