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

Tracking issue: require(esm) #52697

Open
8 of 9 tasks
joyeecheung opened this issue Apr 25, 2024 · 31 comments
Open
8 of 9 tasks

Tracking issue: require(esm) #52697

joyeecheung opened this issue Apr 25, 2024 · 31 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Apr 25, 2024

Before it's unflagged

  • Figure out default export interop with transpilers, either adding __esModule to required ESM on our end (module: add __esModule to require()'d ESM #52166), or transpilers update themselves to check the result returned by require():
  • conditional exports for module, regardless of whether it's loaded by require or import. Something like module which is recognized by Webpack and Rollup would be good (maybe this doesn't need to block unflagging, but should be done before stablization) module: implement the "module-sync" exports condition #54648
  • Move experimental warning to where require() is actually handling a ESM

Before it is promoted to be stable:

Nice-to-haves:

Bug fixes & changes:

Related features that interoperate with require(esm) and need to be considered when being backported together:

For v22.x backport (see a summary of regression analysis in #55217 (comment))

For v20.x backport: TBD

@joyeecheung joyeecheung added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Apr 25, 2024
@GeoffreyBooth
Copy link
Member

@nodejs/loaders

@jakebailey
Copy link

I just wanted to note it here, but it would be super super awesome if (once stable) this were backported to Node 20/22 or even Node 18 if still in support. I'd love to be able to propose a change to switch TypeScript to ESM (given I have it working without breaking CJS consumers), but the time horizon of Node 22 being the oldest supported version is pretty daunting.

It also seems like there is a hacky way using multiple entrypoints that could allow for TS to grab Node's builtins conditionally without #52599/#52762, though none of that is possible without require(ESM), of course.

Even without TypeScript's use case, I think the feature itself is a really important one for the ecosystem. Backporting would really make ESM changeovers a lot less painful.

@Andarist
Copy link
Contributor

Andarist commented May 7, 2024

IIRC from some Twitter threads - there is a plan to backport this once the feature stabilizes.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 11, 2024

Regarding the conditional exports, @guybedford suggested to implement just the module condition that Rollup and Webpack already recognize. I have a WIP but need to do some testing which involves many combinations of test cases 😵‍💫 but will also do some npm crawling to see if it can be picked up/is in conflict with any existing popular packages.

@GeoffreyBooth
Copy link
Member

@guybedford suggested to implement just the module condition that Rollup and Webpack already recognize.

The module top level field, or within exports?

Personally I think require-module makes more sense, as it would complement the existing require and import keys that Node supports.

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 29, 2024

Opened PR for "module" in #54648

Personally I think require-module makes more sense, as it would complement the existing require and import keys that Node supports.

If we are starting from scratch, yes, but then the "module" condition has already been adopted by bundlers that support require(esm) in the wild, so it seems better to go along with the existing convention. See https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62

nodejs-github-bot pushed a commit that referenced this issue Sep 25, 2024
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
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]>
nodejs-github-bot pushed a commit that referenced this issue Sep 26, 2024
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
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]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Oct 1, 2024
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: nodejs#54648
Fixes: nodejs#52173
Refs: https://github.com/joyeecheung/test-module-condition
Refs: nodejs#52697
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Oct 1, 2024
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: nodejs#55085
Refs: nodejs#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]>
@voxpelli
Copy link

voxpelli commented Oct 3, 2024

Raised a question on Twitter to @joyeecheung on my conclusions in https://github.com/voxpelli/investigation-esm-require where it seems like Node 22.9.0 may unintentionally allow some ESM-files to be loaded even without the flag: https://twitter.com/voxpelli/status/1841818608713826693

Mentioning here for sake of completeness, if deemed a correct observation a proper issue will be created

@voxpelli
Copy link

voxpelli commented Oct 4, 2024

The above resulted in a PR to fix it: #55250

targos pushed a commit that referenced this issue Oct 4, 2024
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
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]>
@targos
Copy link
Member

targos commented Oct 5, 2024

It looks like npm itself triggers the warning:

$ make lint
cd tools/eslint && if [ -x ""/Users/mzasso/git/nodejs/node/node"" ] && [ -e ""/Users/mzasso/git/nodejs/node/node"" ]; then ""/Users/mzasso/git/nodejs/node/node"" /Users/mzasso/git/nodejs/node/./deps/npm/bin/npm-cli.js ci; elif [ -x `command -v node` ] && [ -e `command -v node` ] && [ `command -v node` ]; then `command -v node` /Users/mzasso/git/nodejs/node/./deps/npm/bin/npm-cli.js ci; else echo "No available node, cannot run \"node /Users/mzasso/git/nodejs/node/./deps/npm/bin/npm-cli.js ci\""; exit 1; fi;
npm warn cli npm v10.8.3 does not support Node.js v23.0.0-pre. This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.
(node:45192) ExperimentalWarning: Support for loading ES Module in require() is an experimental feature and might change at any time
    at emitExperimentalWarning (node:internal/util:269:11)
    at loadESMFromCJS (node:internal/modules/cjs/loader:1388:5)
    at Module._compile (node:internal/modules/cjs/loader:1517:5)
    at Module._extensions..js (node:internal/modules/cjs/loader:1672:16)
    at Module.load (node:internal/modules/cjs/loader:1328:32)
    at Module._load (node:internal/modules/cjs/loader:1138:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:218:24)
    at Module.require (node:internal/modules/cjs/loader:1350:12)
    at require (node:internal/modules/helpers:138:16)

added 185 packages, and audited 186 packages in 8s

41 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
Running JS linter...

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 5, 2024

Yes, because of debug, which I covered in #55217 (comment) and addressed in the last commit of the backport PR. In the last TSC meeting we agreed to silence the warning when require() comes from node_modules on v22 and keep it on v23 to help people notice and update them (with #55241)

joyeecheung added a commit to joyeecheung/node that referenced this issue Oct 7, 2024
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: nodejs#55085
Refs: nodejs#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]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Oct 8, 2024
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: nodejs#55085
Refs: nodejs#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]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Oct 9, 2024
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: nodejs#55085
Refs: nodejs#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]>
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
This patch:

1. Adds ESM syntax detection to compileFunctionForCJSLoader()
  for --experimental-detect-module and allow it to emit the
  warning for how to load ESM when it's used to parse ESM as
  CJS but detection is not enabled.
2. Moves the ESM detection of --experimental-detect-module for
  the entrypoint from executeUserEntryPoint() into
  Module.prototype._compile() and handle it directly in the
  CJS loader so that the errors thrown during compilation *and
  execution* during the loading of the entrypoint does not
  need to be bubbled all the way up. If the entrypoint doesn't
  parse as CJS, and detection is enabled, the CJS loader will
  re-load the entrypoint as ESM on the spot asynchronously using
  runEntryPointWithESMLoader() and cascadedLoader.import(). This
  is fine for the entrypoint because unlike require(ESM) we don't
  the namespace of the entrypoint synchronously, and can just
  ignore the returned value. In this case process.mainModule is
  reset to undefined as they are not available for ESM entrypoints.
3. Supports --experimental-detect-module for require(esm).

PR-URL: #52047
Backport-PR-URL: #56927
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Refs: #52697
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: #52868
Backport-PR-URL: #56927
Fixes: #52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Refs: #52697
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
PR-URL: #53050
Backport-PR-URL: #56927
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Refs: #52697
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
PR-URL: #54868
Backport-PR-URL: #56927
Fixes: #54773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Refs: #52697
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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]>
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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]>
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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]>
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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]>
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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]>
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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
marco-ippolito pushed a commit that referenced this issue Feb 11, 2025
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

10 participants