module: support require()ing synchronous ESM graphs #51977
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
GeoffreyBooth
added
module
RafaelGSS
added
the
notable-change
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
RafaelGSS
added
the
semver-minor
github-actions
bot
removed
the
request-ci
joyeecheung
force-pushed
the
require-esm
branch
2 times, most recently
from
5a27731
to
612b870
Compare
|
Before we get into the technical details, I just want to give a heartfelt THANK YOU to @joyeecheung for taking this on, and express my awe of her brilliance in figuring out how to achieve it. |
I think the hooks do affect
What does this mean? Doing the extension searching for .cjs and/or .mjs in the filename? I wouldn’t worry about that for this PR; anyone doing |
|
I LOVE this idea. It will simplify so many things. Let's keep going with it. |
joyeecheung
force-pushed
the
require-esm
branch
2 times, most recently
from
515d02d
to
1ab2592
Compare
github-actions
bot
removed
the
request-ci
They only affect the
It means I don't know what happens when this happens, and there are not yet any test for it. |
|
Big +1 on the idea and I think bun shows this is feasible and users like it. |
joyeecheung
force-pushed
the
require-esm
branch
2 times, most recently
from
b4c3f5c
to
9905e2d
Compare
joyeecheung
force-pushed
the
require-esm
branch
from
9905e2d
to
a235a15
Compare
|
I think this is backportable to v20. It should go out in v22 next month, too (it's approaching April, I don't know if there's going to be any more v21 minor release). v18 is in maintenance mode so it largely is up to the release team. If you want to try it out before a full release, you can use the nightlies since 20240319: https://nodejs.org/download/nightly/ |
|
Fantastic work - and so many thanks for getting to the bottom of the cjs/esm craziness that is starting to hold Node.js back. Hope this gets into a live version ASAP since it will still take some time for the versions in production systems to roll over. |
|
Thank you joyeecheung for the excellent work. This PR will solve a lot of problems. We no longer need to explain the issues with CJS/ESM to others, and we don't have to worry about various compatibility configurations anymore. |
PR-URL: nodejs#51977 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
This patch adds `require()` support for synchronous ESM graphs under
the flag `--experimental-require-module`
This is based on the the following design aspect of ESM:
- The resolution can be synchronous (up to the host)
- The evaluation of a synchronous graph (without top-level await) is
also synchronous, and, by the time the module graph is instantiated
(before evaluation starts), this is is already known.
If `--experimental-require-module` is enabled, and the ECMAScript
module being loaded by `require()` meets the following requirements:
- Explicitly marked as an ES module with a `"type": "module"` field in
the closest package.json or a `.mjs` extension.
- Fully synchronous (contains no top-level `await`).
`require()` will load the requested module as an ES Module, and return
the module name space object. In this case it is similar to dynamic
`import()` but is run synchronously and returns the name space object
directly.
```mjs
// point.mjs
export function distance(a, b) {
return (b.x - a.x) ** 2 + (b.y - a.y) ** 2;
}
class Point {
constructor(x, y) { this.x = x; this.y = y; }
}
export default Point;
```
```cjs
const required = require('./point.mjs');
// [Module: null prototype] {
// default: [class Point],
// distance: [Function: distance]
// }
console.log(required);
(async () => {
const imported = await import('./point.mjs');
console.log(imported === required); // true
})();
```
If the module being `require()`'d contains top-level `await`, or the
module graph it `import`s contains top-level `await`,
[`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users
should load the asynchronous module using `import()`.
If `--experimental-print-required-tla` is enabled, instead of throwing
`ERR_REQUIRE_ASYNC_MODULE` before evaluation, Node.js will evaluate the
module, try to locate the top-level awaits, and print their location to
help users fix them.
PR-URL: nodejs#51977
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
|
Amazing work @joyeecheung 🚀 This will be a huge boon for the current RedwoodJS development roadmap.
If there's any way it is (and any way we can be of help), we'd make use of this immediately. |
|
Oh my... importing ESMs from CJS? I didn't think I'd see the day. Node team: how long before this is not behind a flag anymore? This will basically kill any reason to hold back the updating of any module to ESM. This is a dream. @joyeecheung ... thank you. Thank you. Thank you. |
|
I was really excited about this feature and wanted to test it, but I can't manage to make it works, even by literally copy/pasting the examples given here and in the documentation of v22.0.0 // point.mjs
export function distance(a, b) { return (b.x - a.x) ** 2 + (b.y - a.y) ** 2; }
class Point {
constructor(x, y) { this.x = x; this.y = y; }
}
export default Point;// app.mjs
const required = require('./point.mjs');
// [Module: null prototype] {
// default: [class Point],
// distance: [Function: distance]
// }
console.log(required);
(async () => {
const imported = await import('./point.mjs');
console.log(imported === required); // true
})();But running this command will fails : I'm pretty sure to have read twice the documentations and fulfilled each requirement to make this works, but I may have miss something, did I ? |
|
I think you have things backwards; this PR enabled writing (I think you pulled the code from the PR description, which doesn't annotate the filename for the code containing |
|
I completely understood it backwards indeed, thank you for the rectification which makes more sense to me now. |
|
I added a // main.js comment to the example in the OP to avoid the confusion. |
Summary
This patch adds
require()support for synchronous ESM graphs underthe flag
--experimental-require-moduleThis is based on the the following design aspect of ESM:
also synchronous, and, by the time the module graph is instantiated
(before evaluation starts), this is is already known.
If
--experimental-require-moduleis enabled, and the ECMAScriptmodule being loaded by
require()meets the following requirements:"type": "module"field inthe closest package.json or a
.mjsextension.await).require()will load the requested module as an ES Module, and returnthe module name space object. In this case it is similar to dynamic
import()but is run synchronously and returns the name space objectdirectly.
If the module being
require()'d contains top-levelawait, or the modulegraph it
imports contains top-levelawait,ERR_REQUIRE_ASYNC_MODULEwill be thrown. In this case, users shouldload the asynchronous module using
import().If
--experimental-print-required-tlais enabled, instead of throwingERR_REQUIRE_ASYNC_MODULEbefore evaluation, Node.js will evaluate themodule, try to locate the top-level awaits, and print their location to
help users find them.
Background
There were some previous discussions about this idea back in 2019 (e.g. #49450). I I didn't go through all of them, but in 2024 I believe we can agree that not supporting
require(esm)is creating enough pain for our users that we should really deprioritize the drawbacks of it. A non-perfect solution is still better than having nothing at all IMO.There was a previous attempt in #30891 which tried to support TLA from the start and thus needed to run the event loop recursively, which would be unsafe and therefore it was closed (synchronous-only
require(esm)was brought up in #30891 (comment) but the PR didn't end up going that way). I have the impression that there were some other attempts before, but non active AFAIK.This PR tries to keep it simple - only load ESM synchronously when we know it's synchronous (which is part of the design of ESM and is supported by the V8 API), and if it contains TLA, we throw. That should at least address the majority of use cases of ESM (TLA in a module that's supposed to be import'ed is already not a great idea, they are more meant for entry points. If they are really needed, users can use
import()to make that asynchronicity explicit).When I was refactoring the module loader implementation and touching the V8 Module API to fix other issues, this idea appears to be natural to me (since ESM is really designed to have this synchronocity in mind) and does not actually need that much work in 2024 (er, with some refactorings that I already did for other issues at least..), so here is another attempt at it.
Motivation
The motivation for this is probably obvious, but I'll give my take again in case there are unfamiliar readers: CJS/ESM interop would always be done on a best-effort basis and they should not be mixed if avoidable, but today the majority of the popular packages out there in the registry are still CJS. There needs to be an escape hatch for simple cases while the transition happens.
With
require(esm), when a dependency goes ESM-only, it is less likely to be a breaking change for users as long as it's a synchronous ESM (with no top-level await), which should be the case most of the time. This helps package authors transition to ESM without worrying about user experience, or having to release it as dual module which bloats thenode_modulessize even further and leads to identity problems due to the duplication.The design of ESM already ensures that synchronous evaluation and therefore interop with CJS for a synchronous graph is possible (e.g. see tc39/proposal-top-level-await#61), and we won't be alone in restricting TLA for certain features(e.g. w3c/ServiceWorker#1407 service workers on the web also disallows TLA) it would be a shame not to make use of that. Ongoing proposal like import defer could also help addressing the lazy-loading needs without breaking the synchronous aspect of ESM.
TODOs
There are still some feature interactions that this implementation doesn't handle (e.g.
--experimental-detect-moduleor--experimental-loaderor--experimental-wasm-modules). Some edge cases involving cycles probably would have undefined behaviors. I don't think this needs to handle interactions with everything (especially other experimental features) perfectly to land as a first iteration of an experimental feature. We can continue iterating on it while it's experimental.