[v20.x] backport require(esm) and related patches #53500
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
lib / src
github-actions
bot
removed
the
request-ci
marco-ippolito
force-pushed
the
v20.x-staging
branch
from
a924e20
to
473fa73
Compare
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
This refactors the code that compiles SourceTextModule for the built-in ESM loader to use a common routine so that it's easier to customize cache handling for the ESM loader. In addition this introduces a common symbol for import.meta and import() so that we don't need to create additional closures as handlers, since we can get all the information we need from the V8 callback already. This should reduce the memory footprint of ESM as well. PR-URL: nodejs#52291 Refs: nodejs#47472 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
This patch disallows CJS <-> ESM edges when they come from require(esm) requested in ESM evalaution. Drive-by: don't reuse the cache for imported CJS modules to stash source code of required ESM because the former is also used for cycle detection. PR-URL: nodejs#52264 Fixes: nodejs#52145 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Symbol properties are typically more GC-efficient than using WeakMaps, since WeakMap requires ephemeron GC. `module[kModuleExportNames]` would be easier to read than `importedCJSCache.get(module).exportNames` as well. PR-URL: nodejs#52095 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#52437 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Feng Yu <[email protected]>
Previously there is an edge case where submodules loaded by require() may not be loaded by import() again from different intermediate edges in the graph. This patch fixes that, added tests, and added debug logs. Drive-by: make loader a private field so it doesn't show up in logs. PR-URL: nodejs#52487 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
joyeecheung
force-pushed
the
backport-require-esm
branch
from
b2e2b51
to
7298d8d
Compare
|
Rebased this branch. It looks like #53502 is getting stalled. This doesn't need to wait for that PR to get merged to v20.x though @marco-ippolito how should we proceed getting this into 20.x? |
If CI is green, Ill include it in the next 20 release |
github-actions
bot
removed
the
request-ci
|
CI was green, should we merge this? |
|
I'll backport it today 💪🏻 |
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 8, 2024
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: #51977
Backport-PR-URL: #53500
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 8, 2024
This refactors the code that compiles SourceTextModule for the built-in ESM loader to use a common routine so that it's easier to customize cache handling for the ESM loader. In addition this introduces a common symbol for import.meta and import() so that we don't need to create additional closures as handlers, since we can get all the information we need from the V8 callback already. This should reduce the memory footprint of ESM as well. PR-URL: #52291 Backport-PR-URL: #53500 Refs: #47472 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 8, 2024
This patch disallows CJS <-> ESM edges when they come from require(esm) requested in ESM evalaution. Drive-by: don't reuse the cache for imported CJS modules to stash source code of required ESM because the former is also used for cycle detection. PR-URL: #52264 Backport-PR-URL: #53500 Fixes: #52145 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 8, 2024
Symbol properties are typically more GC-efficient than using WeakMaps, since WeakMap requires ephemeron GC. `module[kModuleExportNames]` would be easier to read than `importedCJSCache.get(module).exportNames` as well. PR-URL: #52095 Backport-PR-URL: #53500 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 8, 2024
PR-URL: #52437 Backport-PR-URL: #53500 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Feng Yu <[email protected]>
|
Landed in 30b859f...7625dc4 |
marco-ippolito
pushed a commit
that referenced
this pull request
Aug 8, 2024
Previously there is an edge case where submodules loaded by require() may not be loaded by import() again from different intermediate edges in the graph. This patch fixes that, added tests, and added debug logs. Drive-by: make loader a private field so it doesn't show up in logs. PR-URL: #52487 Backport-PR-URL: #53500 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This backports #51977 and related bug fixes. To reduce the amount of conflicts some other patches touching adjacent lines are also backported.
Note that
src: backport FromV8Arrayis a special backport of #51758 which implements the internal API using the old array iteration approach, because the new V8 API is not available in v20.x - so it's still not going to get any benefit from the new API, but at least this helps reducing the source conflicts when backporting future commits that make use of the new faster callback-based array iteration API.Module format detection is not backported because all the recent changes in format detection on the main branch are built on top of #50322. If #50322 is not backported first, rewriting the later changes to use the old internal API is going to make backporting it out-of-order even harder. But since that PR is pretty big and adds a new dependency it seems less risky to just backport that separately.