-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
module: implement the "module-sync" exports condition #54648
Conversation
Review requested:
|
cc @guybedford |
ee9bdea
to
89c1e4f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document the subtle difference between "module" and "import" at https://nodejs.org/api/packages.html#conditional-exports?
There is a huge section in |
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. |
It would be great if the docs mentioned that |
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 You can find some explanation on this in https://github.com/graphql/graphq where we are exploring to ship CommonJS for 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @phryneas !
I confirm that this PR breaks require('xstate')
under --experimental-require-module
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to block.
How did you confirm that this breaks 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, ".": {
"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 |
Do you have an example? Considering bundlers have been using this as an optimization for bundling code that does |
I'm no longer with my computer but my reproduction ended up in a throw (while v22 with the flag doesn't throw). |
b8a4492
to
bb65ea2
Compare
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
bb65ea2
to
e96699e
Compare
Failed to start CI⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/10997539161 |
@guybedford @legendecas @JakobJingleheimer @targos Can you take a look again? Thanks |
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. |
Landed in 0b9249e |
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 <jacob@frende.me> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
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 on newer versions of Node.js that supports require(esm) to avoid the dual-package hazard.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)
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'
orimport './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
module
condition from webpack and esbuild bundles, will a small difference in rollup outputs. See https://github.com/joyeecheung/test-module-conditionFixes: #52173
Refs: https://github.com/joyeecheung/test-module-condition
Refs: #52697
Refs: https://webpack.js.org/guides/package-exports/#target-environment-independent-packages