module: implement the "module-sync" exports condition

[joyeecheung]

This patch implements a "module-sync" exports condition that's enabled when require(esm) is enabled, so that packages can supply a sycnrhonous ES module to the Node.js module loader, no matter it's being required or imported. (Currently require(esm) is still under --experimental-require-module on v22. It will be unflagged in the upcoming v23 release first. We expect to also unflag require(esm) in v22 after testing in 23).

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 on newer versions of Node.js that supports require(esm) to avoid the dual-package hazard.

{
  "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-sync" and require(esm) are
      // not supported, use the CJS version to avoid dual-package hazard. 
      // When package authors think it's time to drop support for older versions of
      // Node.js, they can remove the exports conditions and just use "main": "index.js".
      "default": "./dist/index.cjs"
    },
    // On any other environment, use the ESM version.
    "default": "./index.js"
  }
}

Or if the package is only meant to be run on Node.js and wants to fallback to CJS on older versions that don't have require(esm)

{
  "type": "module",
  "exports": {
    // 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-sync" and require(esm) are
    // not supported, use the CJS version to avoid dual-package hazard. 
    // When package authors think it's time to drop support for older versions of
    // Node.js, they can remove the exports conditions and just use "main": "index.js".
    "default": "./dist/index.cjs"
  }
}

For package authors: this only serves as a feature-detection mechanism for packages that wish to support both CJS and ESM users during the period when some active Node.js LTS versions support require(esm) while some older ones don't. When all active Node.js LTS lines support require(esm), packages can simplify their distributions by bumping the major version, dropping their CJS exports, and removing the module-sync exports condition (with only main or default targetting the ESM exports). If the package needs to support both bundlers and being run unbundled on Node.js during the transition period, use both module-sync and module and point them to the same ESM file. If the package already doesn't want to support older versions of Node.js that doesn't support require(esm), don't use this export condition.

For bundlers/tools: they should avoid implementing this stop-gap condition. Most existing bundlers implement the de-facto bundler standard module exports condition, and that should be enough to support users who want to bundle ESM from CJS consumers. Users who want both bundlers and Node.js to recognize the ESM exports can use both module/module-sync conditions during the transition period, and can drop module-sync+module when they no longer need to support older versions of Node.js. If tools do want to support this condition, it's recommended to make the resolution rules in the graph pointed by this condition match the Node.js native ESM rules to avoid divergence.

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.

The tests added in this patch match the output of using the modulecondition from webpack and esbuild bundles, will a small difference in rollup outputs. See https://github.com/joyeecheung/test-module-condition

Fixes: #52173
Refs: https://github.com/joyeecheung/test-module-condition
Refs: #52697
Refs: https://webpack.js.org/guides/package-exports/#target-environment-independent-packages

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[joyeecheung]

cc @guybedford

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (e35299a) to head (e96699e).
Report is 199 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54648      +/-   ##
==========================================
+ Coverage   88.03%   88.24%   +0.21%     
==========================================
  Files         652      652              
  Lines      183797   183886      +89     
  Branches    35863    35853      -10     
==========================================
+ Hits       161804   162279     +475     
+ Misses      15242    14896     -346     
+ Partials     6751     6711      -40     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/get_format.js	89.28% <100.00%> (ø)
lib/internal/modules/esm/utils.js	98.89% <100.00%> (+<0.01%)	⬆️
lib/internal/modules/helpers.js	99.03% <100.00%> (+0.98%)	⬆️

... and 98 files with indirect coverage changes

[legendecas]

Should we document the subtle difference between "module" and "import" at https://nodejs.org/api/packages.html#conditional-exports?

[joyeecheung]

There is a huge section in packages.md that teaches some questionable practices (see #52174) that will soon be outdated when we unflag --experimental-require-module. I am not sure if we should just update that doc now, or leave a TODO (I am inclined to just leave a TODO and don't let this get held back by a huge doc bikeshedding - IMO this documentation doesn't belong in the API docs at all, it should probably be placed in some example repo like the n-api examples that are easier to update and less abstract when it describes multi-file structures)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[joyeecheung]

I did a bit more testing with other bundlers, and put the results in https://github.com/joyeecheung/test-module-condition

In summary it allows running unbundled code that produce the same output as bundled produced by webpack and esbuild. Rollup has a difference in that it doesn't respect insertion-order priorities, but I guess that's probably a problem that can apply to any other exports conditions.

[nicolo-ribaudo]

It would be great if the docs mentioned that module is not a catch-all for ESM, but that you should only use it when your ESM does not use top-level await.

[phryneas]

This could be problematic where packages ship build explicitly for bundlers, but not for node.

Just as an example: there is a good bunch of packages that use module because it does not get picked up by node, in elaborate schemes to get around a dual package hazard.
Pinging @Andarist on this, as he is shipping a lot of those packages.

You can find some explanation on this in https://github.com/graphql/graphq where we are exploring to ship CommonJS for require, and stubs re-exporting that CommonJS for imports.
module is used explicitly targeting bundlers with a completely different build.

I do not think that this exact bundling schema would be impacted negatively (as this PR would also get around the dual package hazard), but I imagine there might be more reasons to have something "not for node" in module.

[targos]

Good point @phryneas !

I confirm that this PR breaks require('xstate') under --experimental-require-module.

[targos]

Meant to block.

[joyeecheung]

I confirm that this PR breaks require('xstate') under --experimental-require-module.

How did you confirm that this breaks require('xstate') though? I tried it out locally, and it's actually working as intended - require('xstate') === await import('xstate') is even returning true (because we don't need to add the __esModule hack for xstate since it has no default exports, but even if it does, all the name exports are still going to be reference equal)

According to graphql/graphql-js#4062, take the base path for example:

    ".": {
      "module": "./dist/xstate.esm.js",  // Original ESM version
      "import": "./dist/xstate.cjs.mjs",  // Re-exports of the transpiled CJS
      "default": "./dist/xstate.cjs.js"  // Transpiled ESM version
    },

Before this PR, import 'xstate' picks up the re-exports of the transpiled CJS while require('xstate') picks up the transpiled CJS. After this PR, both of them gets the original ESM version under --experimental-require-module (although this reminds me to fill the gap, we should also make it flagged for import). This is actually the desirable situation, and basically the point of choosing module. If we end up inventing a new condition, say require-module, then the package has to do this if they want to ask newer versions of Node.js to use ESM for require()

    ".": {
      "require-module": "./dist/xstate.esm.js",  // Original ESM version
      "module": "./dist/xstate.esm.js",  // Original ESM version
      "import": "./dist/xstate.cjs.mjs",  // Re-exports of the transpiled CJS
      "default": "./dist/xstate.cjs.js"  // Transpiled ESM version
    },

This further bloating is what we are trying to avoid by implementing module, as bundlers already have intended it to be an optimization for require(esm) on Node.js targets.

[joyeecheung]

I imagine there might be more reasons to have something "not for node" in module.

Do you have an example? Considering bundlers have been using this as an optimization for bundling code that does require(esm) on Node.js, it seems risky for a module to deviate from the intended use case already. The cost of using a new condition name for this is that we will risk further bloating the package.json as described in #54648 (comment), instead of keeping existing package.json work out of the box if they are following a guideline such as https://webpack.js.org/guides/package-exports/#target-environment-independent-packages

[targos]

I'm no longer with my computer but my reproduction ended up in a throw (while v22 with the flag doesn't throw).
Just install xstate, write a commonjs file that requires it, and run the file with the node built from this PR.
Without the flag it passes, with the flag it throws.

[nodejs-github-bot]

Landed in 0b9249e

[joyeecheung]

Adding don't land labels for release branches, because this as a feature detection thing must be released together with #55085 which require manual backports.

[joyeecheung]

Actually this commit was landed behind the CLI flag, so the code could be released first, though we should mention in the release notes that it's only enabled when require(esm) is enabled. I updated the OP text for releasers.