Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common: Make Export non-default / ESM Module Support #1632

Closed
holgerd77 opened this issue Jan 13, 2022 · 14 comments
Closed

Common: Make Export non-default / ESM Module Support #1632

holgerd77 opened this issue Jan 13, 2022 · 14 comments

Comments

@holgerd77
Copy link
Member

holgerd77 commented Jan 13, 2022

Part of #1024

Target branch: develop
Assigned to: ScottyPoi

Our main object from the Common (for setting the chain/HF state) is exported as a default export. This causes problems like #978 and therefore the default export should be replace by a named export (by simply removing the default keyword).

This will need a broad set of adoption in the consuming code base (other monorepo libraries), some solid half-manual search-and-replace should help here a lot though. A full automated search-and-replace though is not possible since a Common import is also referenced a lot in e.g. CHANGELOG files, where examples should not be changed. In associated README files examples though should be adopted to the new import pattern, I guess just:

import { Common } from '@ethereumjs/common'

Note that this PR should also branch off from develop, likely easy to be forgotten.

[ CURRENTLY BLOCKED, SEE THREAD BELOW ]

@ryanio
Copy link
Contributor

ryanio commented Jan 13, 2022

Hmm, I thought a default export is rather standard these days (we just added one to rlp), I'd rather find a solution that allows us to work with esm simultaneously rather than remove default exports across our libs?

If we output esm module code correctly, can default exports still be used?

I'll try some experiments on this today and report back here.

It's also okay to have both default and non-default exports as we do today. So I can try to add Common as a normal export and see if it can bypass the default export if that solves the mjs issues. It sounds like we might need a dedicated mjs build to support file extensions though. I'll read more.

@holgerd77
Copy link
Member Author

@ryanio ok, thanks for chiming in, then let's put this on hold.

I thought that was the consensus along some of the discussions, but I wouldn't take for myself that I would have completely internalized the topic.

@ryanio
Copy link
Contributor

ryanio commented Jan 13, 2022

This is super interesting stuff to me and I will continue to update here with my findings.

This page has a lot of information: https://www.typescriptlang.org/docs/handbook/esm-node.html

I am thinking one solution would be to make tsconfig.prod.esm.json that outputs to dist.esm and then use the "exports" field as explained in https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

@holgerd77 I'll test if the "browser" field would still be respected for the es5 build at dist.browser. If so that would be great, and we could possibly include this update as part of all packages' patch and minor versions since it shouldn't be breaking in any way.

I'm not sure if the esm support has landed in our current TS version though or if it's still only available on nightly as the notice on top of the handbook page link above currently suggests.

@ryanio
Copy link
Contributor

ryanio commented Jan 15, 2022

Ok this is what I've tried so far.

I started by updating typescript to "next" and creating packages/common/tsconfig.esm.json:

{
  "extends": "./tsconfig.prod.json",
  "compilerOptions": {
    "target": "es2020",
    "module": "node12",
    "moduleResolution": "node12",
    "outDir": "./dist.esm",
  },
}

Then added this file to the build script.

This would be added to package.json:

  "type": "module",
  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "import": "./dist.esm/index.js",
      "require": "./dist/index.js"
    }
  },

Lots of errors on build now. Here are the two main ones:

src/chains/index.ts:1:35 - error TS2835: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node12' or 'nodenext'. Did you mean './../types.js'?

1 import { Chain, chainsType } from './../types'

There is a lot of interesting discussion about this.

Basically, esmodules requires file extensions (boo) so all ts file imports have to be named with '.js'.

It seems like this is a hard requirement with no flexibility to be changed in the future (microsoft/TypeScript#42151 (comment))

src/chains/index.ts:2:21 - error TS7062: JSON imports are experimental in ES module mode imports.

2 import mainnet from './mainnet.json'

This one is confusing to me. I understand that json modules should still use require() on the backend, but I figure typescript would take care of transpiling this, so that we use this syntax to import json files with type support. I think the solution here would be to use the syntax import mainnet = require('./mainnet.json').


These are the only two issues so far (the others were duplicative of these two core reasons), but that's just for common which is a relatively light package, so there might be more things in bigger packages like vm or client. Regardless there would be 100s of lines to fix for the two errors above across the monorepo alone.

@holgerd77 holgerd77 changed the title Common: Make Export non-default Common: Make Export non-default / ESM Module Support Jan 17, 2022
@holgerd77
Copy link
Member Author

@ryanio thanks for digging in so deep here. 🙏 I have to admit, this is not my favorite problem space. 😋 😬 Maybe we can do some exchange on this during the team call.

@ryanio
Copy link
Contributor

ryanio commented Jan 20, 2022

have opened: #1654

@paulmillr
Copy link
Member

My 2c-s on this issue: would be great to just switch to ESM, instead of providing cjs/esm double-builds.

That's 2x less code to include in npm installs, and 2x less cases to support. Most stuff supports esm these days. Also cjs modules can be imported from esm, but not wise versa.

@MicahZoltu
Copy link
Contributor

Agree on just using ESM everywhere.

export default should generally be avoided because it has poor developer ergonomics compared to exporting individual items.

rlp currently doesn't work natively in browser because there is no ESM build (which it sounds like this issue is targeting), which means @paulmillr's micro-eth-signer doesn't work natively in the browser (rlp is the only dependency and hence the only thing holding it back from working natively in a browser).

@paulmillr
Copy link
Member

paulmillr commented Mar 12, 2023

I want to make noble and ethereum-cryptography v2 ESM-only. Before that, it's important to understand the impact of such decision on all users; such as the monorepo.

The idea is to make v7 also pure ESM. See the gist by Sindre (largest NPM package developer out there who moved to ESM) detailing move and problems of hybrid approach. The pure-ESM move is easily reversible and monorepo could always move back to hybrid cjs/esm approach in 7.0.1 if that doesn't work.

On a glance, the decision may seem bold. However:

  1. All browsers and stable node.js versions support ESM
  2. Bundler-less future: Common.js code can't be used in browsers, Deno, other environments without bundlers. ESM code can be used natively in browsers without any hassle, bundlers, builders, etc.
  3. Pure-ESM dependencies can't be used from common.js code. Meaning, it's viral. Meaning, at some point, some user dependency would update to ESM and they would also need to update. So it's just a matter of time until everyone switches.
  4. There are some environments that don't support ESM: Electron, Hardhat. Hardhat folks told me they wanted to upgrade in the next release. Electron, however, won't support it even in 2025, seemingly: the problem resides in their architecture, it's too complex for an easy switch. Electron folks tell everyone to use a bundler to compile ESM code to common.js, which means that won't impact them.
  5. It's exhausting to maintain two module systems and it's not always correct. Package size is doubled, and with source maps that could become an issue.
  6. Static analysis benefits: all imports and exports must be defined at "compile" time instead of dynamically at runtime, allow for better static analysis and tooling (think tree-shaking, dead code elimination)
  7. Doing so will ensure we have a clear path towards eliminating usage of import * from "crypto". It is important to do: even pure-ESM cryptography pkg won't work in browsers as-is, without bundlers. Built-in webcrypto global should be used instead. It is already supported in browsers and deno; node.js would need to deprecate v18 for this to happen - but a simple shim would save the day for older node.js versions.

Questions to monorepo maintainers:

  1. What are your arguments in favor of keeping common.js, if we resolve issues with main consumers (meaning Hardhat)?
  2. Can this be done in v7 instead of delaying it 1 year further? I think v7 is a great target because it removes Buffer. Making code importable in a browser is awesome.

@paulmillr
Copy link
Member

Interesting, hardhat seems to already support ESM as per last week: https://github.com/NomicFoundation/hardhat/releases/tag/hardhat%402.13.0

cc @alcuadrado is TypeScript the only thing that doesn't support ESM with Hardhat?

@alcuadrado
Copy link
Member

Nop, unfortunately Hardhat's core depends on CJS pretty heavily.

The reason for that is that our plugin system is dependent on the order of require() that users make to plugins. In ESM the order of imports is not guaranteed (and does change in practice).

We need to rework our plugin system for it to fully support ESM.

I'm starting to think that maybe we should move to esm-only when it's properly supported by ts-node or some similar project. But note that even after that, the migration would be a pretty large project, as every plugin will need to be adapted.

@chaitanyapotti
Copy link
Contributor

chaitanyapotti commented May 6, 2023

Glad that this is being prioritised. Right now, tree-shaking is not straightforward with cjs builds being exported.
In the meantime, it would help if module/es build is also exported along with cjs. I can do a quick PR for that if necessary.

@holgerd77
Copy link
Member Author

Have given this a re-start for the upcoming breaking releases (broader context: current release work is taking place on develop-v7, will soon merged into master, then 2-3 alpha/pre releases during the next weeks, then release in June/July (rather July)), taking a road with a hybrid approach:
#2685

Feedback appreciated! 🙂

@acolytec3
Copy link
Contributor

We both do named exports and esm builds now courtesy of @holgerd77 so v7 will include ESM. Woot woot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants