Monorepo: Switch to hybrid ESM/CJS Build

[holgerd77]

I gave the ESM build a try this morning, is this basically how we want to do it?

I took inspiration from the following two repositories, since both have recent builds and a perceived solid and simple pipeline and we used the code recently:

• https://github.com/isaacs/node-lru-cache, package.json
• https://github.com/js-sdsl/js-sdsl

Block has no subfolders, so for the subfolder including packages (so: like src/consensus for Blockchain) we then need to adopt import/export paths like being done here e.g. for the js-sdsl package, do I get this right?

Early feedback appreciated! 😊

[acolytec3]

That sounds right. Every import has to be exactly specified, so if we don't re-export a subfolder from the root level index.js, you have to specify the exact path relative to the package root.

[codecov]

Codecov Report

Merging #2685 (8e70475) into master (3b25d56) will decrease coverage by 0.19%.
The diff coverage is 70.29%.

Additional details and impacted files

Flag	Coverage Δ
block	90.30% <ø> (ø)
blockchain	90.50% <100.00%> (-0.05%)	⬇️
client	86.98% <85.71%> (+<0.01%)	⬆️
common	96.06% <ø> (ø)
devp2p	89.47% <69.67%> (-2.43%)	⬇️
ethash	∅ <ø> (∅)
evm	79.37% <ø> (ø)
rlp	∅ <ø> (∅)
statemanager	80.92% <ø> (ø)
trie	89.94% <ø> (-0.29%)	⬇️
tx	95.50% <ø> (ø)
util	81.34% <100.00%> (+<0.01%)	⬆️
vm	81.36% <ø> (ø)

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

[MicahZoltu]

Along with the changes here (which generally look good, see feedback below) you will also need to update all of your code (potentially in a separate PR) to have explicit full file paths. For example, in packages/block/src/index.ts you should change

export { Block } from './block'
export { BlockHeader } from './header'

to

export { Block } from './block.js'
export { BlockHeader } from './header.js'

Anywhere in the project you refer to a folder (where NodeJS will implicitly load a file named index.js), you should include the full path to the file being loaded. For example, in packages/evm/src/evm.ts you should change

import type { OpHandler, OpcodeList } from './opcodes'

to

import type { OpHandler, OpcodeList } from './opcodes/index.js'

---

I don't know if it applies to this project, but if you want the users of this library to be able to do subpath imports like import { ... } from '@ethereumjs/evm/opcodes' then you will need to explicitly declare each valid subpath in the package.json. You can see the NodeJS docs on subpath imports here: https://nodejs.org/api/packages.html#subpath-imports

[holgerd77]

@MicahZoltu thanks a lot for the early review and the detailed write-up, appreciate this a lot!! 🙏

Regarding file extension and path references, think three topics here to handle separately:

1. File Extensions: js-sdsl (which I take as some orientation frame) is also not adding file extensions like .js, and I have also checked in their build (by going into my own local node_modules folder), there is also nothing added along the build process. So I wonder if this is (still) necessary or not, maybe @ZLY201 (from js-sdsl) can also chime in here and report from the js-sdsl experiences? Also couldn't completely read between the lines during a quick first look from issues like Adding support for ESM references without a .js extension  nodejs/node#46006 what is the latest "state of the art" here and what the implications are. 😋 So for now I would skip until I have more clarity.

2. Path references in imports and exports: I was also planning to go the js-sdsl way here (see screenshot above), they are doing ./subfolder1/subfolder2 for subfolder references and @/folder1/folder2 for higher level references. Has this some advantages or disadvantages?

3. Subpath imports: TBH I do not have the complete overview right now. I would very much assume that people are doing subfolder imports right now here and there. Since we are doing breaking releases it might be an occasion anyhow to get away from that and export thing which should be used through index.ts. So will skip anything to package.json in this regard for now.

[ZLY201]

@MicahZoltu thanks a lot for the early review and the detailed write-up, appreciate this a lot!! 🙏

Regarding file extension and path references, think three topics here to handle separately:

1. File Extensions: js-sdsl (which I take as some orientation frame) is also not adding file extensions like .js, and I have also checked in their build (by going into my own local node_modules folder), there is also nothing added along the build process. So I wonder if this is (still) necessary or not, maybe @ZLY201 (from js-sdsl) can also chime in here and report from the js-sdsl experiences? Also couldn't completely read between the lines during a quick first look from issues like Adding support for ESM references without a .js extension  nodejs/node#46006 what is the latest "state of the art" here and what the implications are. 😋 So for now I would skip until I have more clarity.
2. Path references in imports and exports: I was also planning to go the js-sdsl way here (see screenshot above), they are doing ./subfolder1/subfolder2 for subfolder references and @/folder1/folder2 for higher level references. Has this some advantages or disadvantages?
3. Subpath imports: TBH I do not have the complete overview right now. I would very much assume that people are doing subfolder imports right now here and there. Since we are doing breaking releases it might be an occasion anyhow to get away from that and export thing which should be used through index.ts. So will skip anything to package.json in this regard for now.

Thanks for your invitation! I'll try to give my experiences for project building.

1. File Extensions: This is a native feature of typescript. See https://www.typescriptlang.org/docs/handbook/module-resolution.html. If you use tsc package your project you can safely use this feature. Just change moduleResolution to node in tsconfig.json.
2. Path references in imports and exports: There are no difference between ./xxx with @/xxx, I use @ because my code IDE auto generate import statement with path references. In other words, this makes my development easier.

[MicahZoltu]

File Extensions: js-sdsl (which I take as some orientation frame) is also not adding file extensions like .js, and I have also checked in their build (by going into my own local node_modules folder), there is also nothing added along the build process. So I wonder if this is (still) necessary or not, maybe @ZLY201 (from js-sdsl) can also chime in here and report from the js-sdsl experiences? Also couldn't completely read between the lines during a quick first look from issues like Adding support for ESM references without a .js extension nodejs/node#46006 what is the latest "state of the art" here and what the implications are. 😋 So for now I would skip until I have more clarity.

If you want your code to run in a browser natively you MUST have exact path imports. While NodeJS and bundlers can scan files on disk to figure out an exact path form a fuzzy path, (e.g., adding /index.js or adding .mjs extension), a browser will not scan for files. When you have import { ... } from './something' the browser will literally fetch https://origin/path/to/file/something and if that is a 404 the load will fail.

While many projects use bundlers to fix libraries that do don't have full paths, it would be far better if the library wasn't broken out of the box and in need of fixing by users of the library. If the intent is for this to only be used in NodeJS then fuzzy I think will still work, but the best practice is still to be explicit so you can work in any runtime that may refuse to do fuzzy path matching (node, deno, ts-node, browsers, etc.)

[MicahZoltu]

2. Path references in imports and exports: I was also planning to go the js-sdsl way here (see screenshot above), they are doing ./subfolder1/subfolder2 for subfolder references and @/folder1/folder2 for higher level references. Has this some advantages or disadvantages?

While I haven't used the @ (and similar) stuff before personally, I have heard complaints/frustration caused by it so my general recommendation is to just avoid them. IIRC TypeScript will not do path rewriting so if you do anything other than a naive relative path you MUST have additional build tools that will do the path rewriting for you. If you don't do the path rewriting, then your library will not run natively in the browser (which I think should be a target/goal for any widely used library).

If you already have some post-compile build tooling this may not be worth changing now, but my general recommendation to projects is to try to avoid becoming heavily dependent on build tooling magic, especially when you could avoid that dependency with a simple behavior change like using file-relative paths rather package-relative paths. By avoiding dependency on build tooling, it gives you more freedom to swap out your build tooling in the future should you desire to, or drop it completely (which is my preference for my personal projects).

[MicahZoltu]

Another note on point 2: If you import a folder rather than a file, you run into the exact same set of problems I mentioned in #2685 (comment), both of these are the same issue ultimately, which is that the browser will interpret import statements literally.

[holgerd77]

@MicahZoltu thanks a lot! 🙏

I am not completely getting it (regarding the file extensions) how this interplays with TypeScript, just tested:

When I added a .js extension to our TypeScript code base (took import { normalizeTxParams } from '../src/fromRpc' in our tx tests) and so add/replace to fromRpc.js test run simply fails with Cannot find module '../src/fromRpc.js'.

When I use .ts for the TypeScript base it complains with "can only allow extension when allowImportingTsExtensions is enabled`.

Should I use .ts then and enable this option? 🤔

Any side effects to expect here?

[holgerd77]

(also, can't TypeScript do this automatically on build? Always add the full file references?)

[ZLY201]

@noname0310 I'm not proficient in this, do you have any good advice? If you have time, you can take a look at this.

[noname0310]

@holgerd77 You can use a typescript transformer to use path aliases or add js extensions to import statements.

Transformers commonly used to use path aliases such as "@/":
https://www.npmjs.com/package/@zerollup/ts-transform-paths/v/1.7.17

There are also Transformers that add .js extensions, but they may need to be modified and used due to defects:
https://github.com/Zoltu/typescript-transformer-append-js-extension

Generally speaking, using transformer or bundler may become a dependent form on that build toolchain, but I think it's almost inevitable for hybrid builds.
Once you use Transformers, you can derive the best code for each platform (cjs and esm for node.js, umd for browser).

[MicahZoltu]

When I added a .js extension to our TypeScript code base (took import { normalizeTxParams } from '../src/fromRpc' in our tx tests) and so add/replace to fromRpc.js test run simply fails with Cannot find module '../src/fromRpc.js'.

Does the file ../src/fromRpc.ts exist in your project (relative to whatever file you put that code in)?

[MicahZoltu]

Generally speaking, using transformer or bundler may become a dependent form on that build toolchain, but I think it's almost inevitable for hybrid builds.

I agree that using a transformer has a tendency to create a dependency on it, but I disagree that it is inevitable for hybrid builds! Prior to Node 20, which no longer requires hybrid builds if you don't care about supporting older versions, all of my libraries were hybrid projects with TypeScript as the only build tool and the libraries worked in NodeJS (CommonJS) and browsers (ESM) natively.

[noname0310]

@MicahZoltu

The problem in this case seems difficult for library users to use esm without any bundlers(like webpack) in a browser environment.
unless we use a typescript transformer to add js extensions or build with a bundler such as rollup.

[MicahZoltu]

The problem in this case seems difficult for library users to use esm without any bundlers(like webpack) in a browser environment.

It is difficult for users of the library only if the library omits extensions and doesn't require path rewriting by the library user. If the library correctly has complete exact relative file paths everywhere, then the library can be used natively in a browser without any bundler.

All this requires is changing import from './foo' to import from './foo.js' or import from './foo/index.js' (depending on context). Everything should work natively if one does that everywhere.

[paulmillr]

What I know:

1. Browsers don't support imports like ethereumjs/utils/trie without import maps. Asking users to use import maps is a hassle.
2. We are probably going to switch to pure-esm modules some time in the future. Pure-esm modules will work in browsers as-is without bundlers.

What I will likely do:

In the pure usesm version of noble cryptography, subpath imports will be changed from @noble/hashes/sha256 to @noble/hashes/sha256.js to work in browsers as-is.

What needs to be decided:

Should v7 have a breaking change switching external interface to extensioned paths, or should the switch be delayed to some v8/v9 whenever pure esm would be ready? Advantage of v7 switch is that users will be able to have less breaking changes in the future. Users will need to be educated about the change, which will take time.

[holgerd77]

Should v7 have a breaking change switching external interface to extensioned paths

@paulmillr TBH I cannot connect the dots what is meant by this. If we switch to these full path + file extension references internally, how does this effect the external interface? 🤔

[MicahZoltu]

I would do it all at once if it were me. If you already have a breaking change planned for ESM, getting everything working well for native browser at once will save people from having to go through two separate adjustments.

[paulmillr]

@holgerd77 the external interface is not related to the changes. I'm just mentioning it just in case.

To summarize, at some point external interfaces will need to add extension, unless we want ESM in-browser users to mess around with import maps.

[holgerd77]

in between work update: will remove the k-bucket internalization by rebase and try to re-integrate today, this had some side effects with now the devp2p tests not running through any more.

Otherwise I'll continue with the dual build integrations.

After all the discussions I think I will skip the folder transition in this PR, eventually do in a second round.

@acolytec3 please just ignore this work for the develop-v7 rebase update, will adjust later on to master.

[holgerd77]

Ready for review. 🙂

(I will skip any paths adoptions in this round)

[holgerd77]

This was intened to leave this comment in the code to ease a subsequent type integration for the internalized KBucket package (see ./ext/kbucket.ts).

[holgerd77]

This was a long battle (2-3+ hours) to get the newly reworked Tx fromProvider() (or similar) tests running which use test-double to replace the real RPC provider.

I tested out lots of alternative test-double invocations, switch tape to use the node ESM loader,... but nothing worked at the end.

So I took this route to open up this provider path in the package.json file here (this is now necessary in case of deep imports for ESM).

I would think though that side effects are limited and this should be acceptable.

[acolytec3]

I'm good with it too, though would you rather me figure out a better way to rewrite the tests? I get the impression that if we're having to explicitly define this provider export just to satisfy a test, it's my test and not our export configuration that should be fixed.

[holgerd77]

It would be great if we can fix the test, I went really deep down this rabbit hole though and tried a lot. But maybe you've got another approach I didn't think of. Not sure if it's worth another 3 hours of work time though.

[acolytec3]

This is looking great! Are you intentionally not building an ESM version of the client? Mainly just curious at this point but we'll need the client to be ESM friendly too if we want to fully upgrade the browser version of it (since libp2p is ESM only now and can't be imported easily in CJS land).

[holgerd77]

This is looking great! Are you intentionally not building an ESM version of the client? Mainly just curious at this point but we'll need the client to be ESM friendly too if we want to fully upgrade the browser version of it (since libp2p is ESM only now and can't be imported easily in CJS land).

Not sure, just thought wouldn't be necessary for now. But if we want to reactivate browser build we can for sure do (maybe we leave for now and do along the browser work? Just weak preference though).

[MicahZoltu]

Perhaps this is what you meant by "paths adoptions"? Switching to ./provider.js is fairly strongly recommended for browser, and even the NodeJS documentation now recommends it.

Suggested change

	"./provider": {
	"import": "./dist/esm/provider.js",
	"require": "./dist/cjs/provider.js"
	}
	"./provider.js": {
	"import": "./dist/esm/provider.js",
	"require": "./dist/cjs/provider.js"
	}

[MicahZoltu]

I don't remember what exactly breaks when doing this, but using non-relative paths can cause you headache/pain down the road:

Suggested change

"baseUrl": "./",

This change may also require updating non-relative URLs to be relative URLs in files in this project.

[acolytec3]

I've removed these baseUrl properties in each of the tsconfigs in devp2p and it doesn't seem to have any impact on our build/tests so will push a commit to clean this up. Thanks for calling it out!

[MicahZoltu]

Same comment as above.

Suggested change

"baseUrl": "./",

[acolytec3]

I say we merge this!

[holgerd77]

Ah, interesting. I would have never gotten to this solution. 🙂

[acolytec3]

It means we're no longer testing the Node16 route for fetching data (using https instead of the fetch API but there are separate tests in util for testing that. The goal here is to test the constructor so this seemed like the least impactful route to that.

[g11tech]

looks good 👍