[v20.x] backport unflagging of require(esm) to v20 #56927

joyeecheung · 2025-02-05T16:54:53Z

This backport includes the following PRs with modifications:

See summary

module: eliminate performance cost of ESM syntax detection for CommonJS entry points #52093: Adapted to the lack of package reader rewrite in v20.x.
module: detect ESM syntax by trying to recompile as SourceTextModule #52413: Adapted to the absence of compile cache support in the C++ layer, borrowing some lines from module: implement NODE_COMPILE_CACHE for automatic on-disk code caching #52535
lib,src: iterate module requests of a module wrap in JS #52058: Do not freeze module.dependencySpecifiers because it's semver-major. Added a FromV8Array internal polyfill for v20.x which does not have the new V8 API.
module: support ESM detection in the CJS loader #52047: pkg?.data.type -> pkg.data?.type because we are not backporting package reader rewriting
module: cache synchronous module jobs before linking #52868: no modifications
module: do not set CJS variables for Worker eval #53050: no modifications
lib: allow CJS source map cache to be reclaimed #51711: Changed location of common.gcUntil in test harness
lib: reduce amount of caught URL errors #52658: Adapted to the lack of import assert naming changes
src: refactor embedded entrypoint loading #53573: No need to take care of compile cache since we are not backporting it to v20
module: add __esModule to require()'d ESM #52166: Adapted to the lack of import assert naming changes. Also v8::ScriptOrigin takes an isolate on v20.
esm: refactor get_format #53872: no modifications
module: unflag detect-module #53619: process.env.TEST_PARALLEL addition in tests is not backported to v20. Doesn't hurt to check it though since it's in the python test runner in v20.
module: do not warn for typeless package.json when there isn't one #54045: Work around absense of types tripping, taking a bit of the typeless package.json warning helper code from module: add --experimental-strip-types #53725
esm: throw ERR_REQUIRE_ESM instead of ERR_INTERNAL_ASSERTION #54868: Removed TypeScript tests since it's not on v20.x
module: remove bogus assertion in CJS entrypoint handling with --import #54592: Use CJSModule._load instead of wrapModuleLoad because we are not backporting the trace events.
module: report unfinished TLA in ambiguous modules #54980: no modifications
module: implement the "module-sync" exports condition #54648: no modifications
module: refator ESM loader for adding future synchronous hooks #54769: Skip onImport.tracePromise because we are not backporting diagnostics channel events. Pass down responseURL in loadAndTranslate because it's needed in v20.x for network import
module: unflag --experimental-require-module #55085: Dropped tests added for type-stripping
module: support 'module.exports' interop export name in require(esm) #54563: no modifications
doc: update require(ESM) history and stability status #55199: fixed version yaml field conflicts in doc
module: use kNodeModulesRE to detect node_modules #55243: isUnderNodeModules() was not in v20.x since we didn't backport typescript changes. Just backported the new version in.
process: add process.features.require_module #55241: Adapted to the lack of test refactoring from process.feature.typescript patch.
src: implement IsInsideNodeModules() in C++ #55286: Port a helper from std::string_view::starts_with to C++17
module: check --experimental-require-module separately from detection #55250: no modifications
module: include module information in require(esm) warning #55397: no modifications
module: allow ESM that failed to be required to be re-imported #55502: lines added have different context in v20.x. Modified to still be the same lines backported.
module: trim off internal stack frames for require(esm) warnings #55496: no modifications
module: fix error thrown from require(esm) hitting TLA repeatedly #55520: lines added have different context in v20.x. Modified to still be the same lines backported.
module: do not warn when require(esm) comes from node_modules #55960: lines added have different context in v20.x and some comment changes are not in v20.x.
module: mark evaluation rejection in require(esm) as handled #56122: no modifications
module: only emit require(esm) warning under --trace-require-module #56194: Context is different because --trace-env flags are not in v20.x. Also dropped TypeScript tests.

The last commit is added to disable require(esm) when policy or network imports are enabled on v20.x - these experimental features that were behind flags have already been removed since v22, so it's not worth the trouble to make them work together on v20.x. Just mark them as unsupported if used together.

I tried to reduce the number of non-essential commits to backport to v20.x to avoid introducing regressions, though the following non-essential and semver-minor changes are still backported because the code is a bit too intertwined. These all have been in v22.x as semver-minor for a few months so it's probably not too risky to backport them to v20.x, either.

Unflagging of detect-module, without it the unflagging of require(esm) can be buggy since the logic of the two are very interwined
~~Support for module loading trace events and diagnostics channel events, these changes are quite invasive in the module loading so not backporting them together would incur a lot of conflicts~~ removed

Refs: #52697

nodejs-github-bot · 2025-02-05T16:54:58Z

Review requested:

@nodejs/loaders
@nodejs/performance
@nodejs/startup

nodejs-github-bot · 2025-02-05T22:41:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/65015/

PR-URL: #53050 Backport-PR-URL: #56927 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: James M Snell <[email protected]> Refs: #52697

marco-ippolito · 2025-02-11T10:58:54Z

Landed in 697a392...ed2d373

Unifies the CJS and ESM source map cache map with SourceMapCacheMap and allows the CJS cache entries to be queried more efficiently with a source url without iteration on an IterableWeakMap. Add a test to verify that the CJS source map cache entry can be reclaimed. PR-URL: #51711 Backport-PR-URL: #56927 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Refs: #52697

PR-URL: #52658 Backport-PR-URL: #56927 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Refs: #52697

This patch: 1. Refactor the routines used to compile and run an embedder entrypoint. In JS land special handling for SEA is done directly in main/embedding.js instead of clobbering the CJS loader. Add warnings to remind users that currently the require() in SEA bundled scripts only supports loading builtins. 2. Don't use the bundled SEA code cache when compiling CJS loaded from disk, since in that case we are certainly not compiling the code bundled into the SEA. Use a is_sea_main flag in CompileFunctionForCJSLoader() (which replaces an unused argument) to pass this into the C++ land - the code cache is still read directly from C++ to avoid the overhead of ArrayBuffer creation. 3. Move SEA loading code into MaybeLoadSingleExecutableApplication() which calls LoadEnvironment() with its own StartExecutionCallback(). This avoids more hidden switches in StartExecution() and make them explicit. Also add some TODOs about how to support ESM in embedded applications. 4. Add more comments PR-URL: #53573 Backport-PR-URL: #56927 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Refs: #52697

Tooling in the ecosystem have been using the __esModule property to recognize transpiled ESM in consuming code. For example, a 'log' package written in ESM: export function log(val) { console.log(val); } Can be transpiled as: exports.__esModule = true; exports.default = function log(val) { console.log(val); } The consuming code may be written like this in ESM: import log from 'log' Which gets transpiled to: const _mod = require('log'); const log = _mod.__esModule ? _mod.default : _mod; So to allow transpiled consuming code to recognize require()'d real ESM as ESM and pick up the default exports, we add a __esModule property by building a source text module facade for any module that has a default export and add .__esModule = true to the exports. We don't do this to modules that don't have default exports to avoid the unnecessary overhead. This maintains the enumerability of the re-exported names and the live binding of the exports. The source of the facade is defined as a constant per-isolate property required_module_facade_source_string, which looks like this export * from 'original'; export { default } from 'original'; export const __esModule = true; And the 'original' module request is always resolved by createRequiredModuleFacade() to wrap which is a ModuleWrap wrapping over the original module. PR-URL: #52166 Backport-PR-URL: #56927 Refs: #52134 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Refs: #52697

PR-URL: #53872 Backport-PR-URL: #56927 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Refs: #52697

PR-URL: #53619 Backport-PR-URL: #56927 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Refs: #52697

It was intended that warnings should only be emitted for an existing package.json without a type. This fixes a confusing warning telling users to update /package.json when there are no package.json on the lookup path at all, like this: [MODULE_TYPELESS_PACKAGE_JSON] Warning: ... parsed as an ES module because module syntax was detected; to avoid the performance penalty of syntax detection, add "type": "module" to /package.json Drive-by: update the warning message to be clear about reparsing and make it clear what's actionable. PR-URL: #54045 Backport-PR-URL: #56927 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Refs: #52697

PR-URL: #54868 Backport-PR-URL: #56927 Fixes: #54773 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Refs: #52697

The synchronous CJS translator can handle entrypoints now, this can be hit when --import is used, so lift the bogus assertions and added tests. PR-URL: #54592 Backport-PR-URL: #56927 Fixes: #54577 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Refs: #52697

PR-URL: #54980 Backport-PR-URL: #56927 Fixes: #54931 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Refs: #52697

This patch implements a "module-sync" exports condition for packages to supply a sycnrhonous ES module to the Node.js module loader, no matter it's being required or imported. This is similar to the "module" condition that bundlers have been using to support `require(esm)` in Node.js, and allows dual-package authors to opt into ESM-first only newer versions of Node.js that supports require(esm) while avoiding the dual-package hazard. ```json { "type": "module", "exports": { "node": { // On new version of Node.js, both require() and import get // the ESM version "module-sync": "./index.js", // On older version of Node.js, where "module" and // require(esm) are not supported, use the transpiled CJS version // to avoid dual-package hazard. Library authors can decide // to drop support for older versions of Node.js when they think // it's time. "default": "./dist/index.cjs" }, // On any other environment, use the ESM version. "default": "./index.js" } } ``` We end up implementing a condition with a different name instead of reusing "module", because existing code in the ecosystem using the "module" condition sometimes also expect the module resolution for these ESM files to work in CJS style, which is supported by bundlers, but the native Node.js loader has intentionally made ESM resolution different from CJS resolution (e.g. forbidding `import './noext'` or `import './directory'`), so it would be semver-major to implement a `"module"` condition without implementing the forbidden ESM resolution rules. For now, this just implments a new condition as semver-minor so it can be backported to older LTS. Refs: https://webpack.js.org/guides/package-exports/#target-environment-independent-packages PR-URL: #54648 Backport-PR-URL: #56927 Fixes: #52173 Refs: https://github.com/joyeecheung/test-module-condition Refs: #52697 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

This lays the foundation for supporting synchronous hooks proposed in nodejs/loaders#198 for ESM. - Corrects and adds several JSDoc comments for internal functions of the ESM loader, as well as explaining how require() for import CJS work in the special resolve/load paths. This doesn't consolidate it with import in require(esm) yet due to caching differences, which is left as a TODO. - The moduleProvider passed into ModuleJob is replaced as moduleOrModulePromise, we call the translators directly in the ESM loader and verify it right after loading for clarity. - Reuse a few refactored out helpers for require(esm) in getModuleJobForRequire(). PR-URL: #54769 Backport-PR-URL: #56927 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]> Refs: #52697

This unflags --experimental-require-module so require(esm) can be used without the flag. For now, when require() actually encounters an ESM, it will still emit an experimental warning. To opt out of the feature, --no-experimental-require-module can be used. There are some tests specifically testing ERR_REQUIRE_ESM. Some of them are repurposed to test --no-experimental-require-module. Some of them are modified to just expect loading require(esm) to work, when it's appropriate. PR-URL: #55085 Backport-PR-URL: #56927 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #54563 Backport-PR-URL: #56927 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Refs: #52697

PR-URL: #55199 Backport-PR-URL: #56927 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Refs: #52697

This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: #55243 Backport-PR-URL: #56927 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Refs: #52697

For detecting whether `require(esm)` is supported without triggering the experimental warning. PR-URL: #55241 Backport-PR-URL: #56927 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Refs: #52697

This previously compiles a script and run it in a new context to avoid global pollution, which is more complex than necessary and can be too slow for it to be reused in other cases. The new implementation just checks the frames in C++ which is safe from global pollution, faster and simpler. The previous implementation also had a bug when the call site is in a ESM, because ESM have URLs as their script names, which don't start with '/' or '\' and will be skipped. The new implementation removes the skipping to fix it for ESM. PR-URL: #55286 Backport-PR-URL: #56927 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Refs: #52697

Previously we assumed if `--experimental-detect-module` is true, then `--experimental-require-module` is true, which isn't the case, as the two can be enabled/disabled separately. This patch fixes the checks so `--no-experimental-require-module` is still effective when `--experimental-detect-module` is enabled. Drive-by: make the assertion messages more informative and remove obsolete TODO about allowing TLA in entrypoints handled by require(esm). PR-URL: #55250 Backport-PR-URL: #56927 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Refs: #52697

When emitting the experimental warning for `require(esm)`, include information about the parent module and the module being require()-d to help users locate and update them. PR-URL: #55397 Backport-PR-URL: #56927 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Refs: #52697

When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: #55502 Backport-PR-URL: #56927 Fixes: #55500 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

Trim off irrelevant internal stack frames for require(esm) warnings so it's easier to locate where the call comes from when --trace-warnings is used. PR-URL: #55496 Backport-PR-URL: #56927 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Refs: #52697

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow. PR-URL: #55520 Backport-PR-URL: #56927 Fixes: #55516 Refs: #52697 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: #55960 Backport-PR-URL: #56927 Refs: #55217 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Previously the implemention of require(esm) only converted the rejected promise from module evaluation into an error, but the rejected promise was still treated as a pending unhandled rejection by the promise rejection callback, because the promise is created by V8 internals and we don't get a chance to mark it as handled, so the rejection incorrectly marked as unhandled would still go through unhandled rejection handling (if no global listener is set, the default handling would print a warning and make the Node.js instance exit with 1). This patch fixes it by calling into the JS promise rejection callback to mark the evalaution rejection handled so that it doesn't go through unhandled rejection handling. PR-URL: #56122 Backport-PR-URL: #56927 Fixes: #56115 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Refs: #52697

require(esm) is relatively stable now and the experimental warning has run its course - it's now more troublesome than useful. This patch changes it to no longer emit a warning unless `--trace-require-module` is explicitly used. The flag supports two modes: - `--trace-require-module=all`: emit warnings for all usages - `--trace-require-module=no-node-modules`: emit warnings for usages that do not come from a `node_modules` folder. PR-URL: #56194 Backport-PR-URL: #56927 Fixes: #55417 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Refs: #52697

These features are already removed since v22. There is no point supporting them to work with require(esm) in v20. PR-URL: #56927 Refs: #52697 Reviewed-By: Marco Ippolito <[email protected]>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. labels Feb 5, 2025

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2025

marco-ippolito pushed a commit that referenced this pull request Feb 11, 2025

module: do not set CJS variables for Worker eval

b86f575

PR-URL: #53050 Backport-PR-URL: #56927 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: James M Snell <[email protected]> Refs: #52697

marco-ippolito closed this Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v20.x] backport unflagging of require(esm) to v20 #56927

[v20.x] backport unflagging of require(esm) to v20 #56927

joyeecheung commented Feb 5, 2025 •

edited

Loading

nodejs-github-bot commented Feb 5, 2025

nodejs-github-bot commented Feb 5, 2025

marco-ippolito commented Feb 11, 2025

[v20.x] backport unflagging of require(esm) to v20 #56927

[v20.x] backport unflagging of require(esm) to v20 #56927

Conversation

joyeecheung commented Feb 5, 2025 • edited Loading

nodejs-github-bot commented Feb 5, 2025

nodejs-github-bot commented Feb 5, 2025

marco-ippolito commented Feb 11, 2025

joyeecheung commented Feb 5, 2025 •

edited

Loading