-
Notifications
You must be signed in to change notification settings - Fork 30k
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: increased surface for hazards with require(esm) experimental flag #52173
Comments
(thanks for moving the discussion from Twitter to GitHub!)
When Joyee tried to make I'm a maintainer of that package so I could easily fix that, but the problem boils down to the Another case where you would not want
This ability to choose priority applies both in the case of just extending
The
Regardless of |
is this a theoretical or that's actually implemented in that flag already? I see your links are still open here and there
there's no hazard if the first
it'll take years, I suppose, but yes. New modules won't be published as dual, old modules with hundred million weekly downloads aren't so "easy-peasy" to change. |
It's reality that export conditions are matched in order, it's theoretical that
I agree, but that's not what already happens today without the flag, so today we are already affected by the problem even without using dynamic import (again, see the docs issue I linked because it contains an example :) ) |
@nicolo-ribaudo this might be a hell of a lucky coincidence but thanks to the fact I use "exports": {
".": {
"types": "./types/index.d.ts",
"import": "./esm/index.js",
"default": "./cjs/index.js"
},
"./package.json": "./package.json"
}, Am I correct in understanding you that this means there's literally nothing I should do to change because I might rest my case if that's the case, still if this is how that flag lands everyone should be aware of this potential issue and be sure modules authors also understand how to solve it ... they have time to just eventually swap export definitions without breaking anything in the meantime, the breaking will eventually land once that flag becomes the default. |
If |
so, the current state is that given this exports definition in the module package that is being either imported or required: {
"name": "whatever",
"type": "module",
"exports": {
".": {
"types": "./types/index.d.ts",
"import": "./esm/index.js",
"default": "./cjs/index.js"
},
"./package.json": "./package.json"
}
} once this flag gets unflagged the only file that will ever be required or imported is edit explicitly: // ./cjs/index.js -> exports.ref = {};
// with flag this points at ./esm/index.js instead
const { ref: cjs } = require("whatever");
// ./esm/index.js -> export const ref = {};
import { ref as esm } from "whatever";
// nothing to see here, it's all good!
console.assert(cjs === esm, "hazards in the house"); If this is the case then my proposal about signaling from module authors what should be preferred is already satisfied ... I just want to be sure this is the case so that at least I don't need to worry about this breaking change in the near future, thank you. |
No, the behavior of that package doesn't currently change under the flag. What I was proposing above is to instead explore make the flag break backwards compatibility, so that in your example both |
of course it does ... that was the whole point, I want it to not fail and force I hope we agree here that make authors able to chose what "future require" should prefer must be a requirement for this flag. |
btw ...
if your point is that it should have a |
No, my point is that today, in sync-only-code, there is no guarantee that a dual package that uses
Yes, I agree! I disagree that this is a breaking change introduced by the flag, but I agree that module authours should have this ability. And I see four concrete ways to do so, I'll list them here. Do nothing This might seem counter intuitive, but I just realized that even if Node.js does nothing you can still publish packages that have the behavior you want. You need to prefix your transpiled CJS files with a
Add a new This would let you have different entrypoints for ESM with TLA (only loaded by {
"name": "foo",
"exports": {
"module-sync": "./foo.mjs",
"default": "./foo.cjs"
}
} let num = Math.random();
export { num }; let num = Math.random();
exports.num = num; Add a new {
"name": "foo",
"exports": {
"import": "./foo.mjs",
"require-esm": "./foo.mjs",
"default": "./foo.cjs"
}
} let num = Math.random();
export { num }; let num = Math.random();
exports.num = num; Change This is a breaking change, but maybe it could be explored. |
P.S. I wouldn't mind a situation like this neither, as ordered JSON is also something not many care about in the wild (AFAIK) {
"name": "whatever",
"type": "module",
"exports": {
".": {
// edit: oops, taken, I meant require-esm
"require": "./esm/index.js",
"types": "./types/index.d.ts",
"import": "./esm/index.js",
"default": "./cjs/index.js"
},
"./package.json": "./package.json"
}
} This adds a new field, it's "scoped" per each defined export, and extremely easy to reason about to me ... we can debate if In my case, when no hazards or side effects are possible (i.e. just utilities and who cares if duplicated), I could decide myself which path should be served and prefer maybe CJS if there's only a default export, translated directly as This would probably be my best option yet because it's also easier for the eyes to understand, and easier for everyone to reason about, imho. |
That EDIT Oh yes, with a new name it's like the |
nowhere in this discussion I am mentioning today ... we all know the today story but it looks we're aligned with possible resolutions. I wouldn't use cryptic or counter-intuitive names for DX sake, About this:
I misunderstood the point in case ... then |
|
apologies then that one I didn't know ... so |
To whom it might concern, I've created a test-case repository that should not fail in either ESM or CJS with the flag enabled: Once that works by just changing |
Am I understanding correctly that the conclusion here is that we can just add a Regarding the other point about supporting TLA in CJS in the OP, would you mind split that out to a different issue? I have the impression that there are already existing feature requests and it would be a lot more technically challenging to implement, considering that:
So I think it's best to decouple an issue that's technically simple to address ( |
That's correct, and if you want to name it
I don't care a single bit about that discussion happening in here, the template to fill out proposals ask alternatives so I've mentioned that alternative but as you pointed out it's been discussed ad nauseam and in my opinion that's the solution to everything in the "migrating CJS to ESM" land but then again, here I am discussing a flag that more than a developer either welcomed or decided it should go in unflagged sooner than later ... hence my loud concerns about the how and the expectations from a module author point of view. We can already ditch that conversation and I am sorry you felt the need, and spent time, to write down that great summary around that issue ... I personally don't care about that issue if impractical, I do care about this new idea/issue if also impractical for modules' authors. I hope this reasons well with you. |
This problem seems to already exist if C were either type of module and did I'm a bit skeptical of |
If you read carefully that's the thing that doesn't happen at all in the real-world and the thing commented as "of course it was possible already" so please do take time to read the whole thing and understand the real-world concern, thank you! |
I have no idea what TLA condition is but all I think is: "why not both?" TLA condition can be handy for everything toolchain related and introspection and the |
Please Consider ...I understand somebody took time to analyze top 30 npm modules but maybe we should not forget npm is the biggest repository that ever was and still is on both the backend side and the front end one. Please do not constrain your assumptions out of top 30 modules and think a bit more broadly, if possible, thank you. |
To quote myself:
If we are talking about the packages I've examined, a closer estimate would probably be around top 200-300 - I skimmed the code of a lot of packages, and if they look vastly similar to others I've seen, I didn't include them in my script. |
I did try to read it and just now re-skimmed trying to find the relevant part, but perhaps I'm confused still |
@joyeecheung it wasn't directed to you, it was a general "please consider" statement ... but on top 30 modules I don't know if FE modules are included but on the Web TLA is expected, it works well, and it's desired. The fact people even think TLA was a mistake is a concern I don't want to even start discussing in here as that involves way more standard groups than just NodeJS. |
So if you follow, nobody is using dynamic This flag allows them to not think about it and finally It was possible before but if analysis is what we are after, and the reason that flag exists at all, is that CJS stuck folks really want to synchronously import ESM and that opens a can of warms all over the graph because those ESM only modules might depend on dual modules behind the scene, and in doing so, they already bring in ESM instead of CJS versions that the previous pure/CJS only codebase already used, and enjoyed, as dual module. |
to @bmeck or others, if you reverse engineer this, it actually makes sense for any ESM only module that depend on a dual module to instead be somehow "poisoned" and import the CJS version of that dual module to satisfy the current main project asking edit I understand this is complicate ... but to me it's an option that might work for the CJS crew and produce the least amount of shenanigans ... if the source of the import/require is CJS, bring in all CJS you can, bring in all ESM you can otherwise |
FYI, even in our dependencies... Lines 200 to 201 in af8ba37
And from the amount of support issues regarding |
I think you are talking about #52174 ? For example, if a dual module makes itself dual by defining both
This is unrelated to But if you follow the suggestion in #52174 and make CJS the default of dual packages in Node.js, this happens:
And the hazard is gone. So I guess the problem comes down to dual modules going ESM-first in Node.js even though ESM can only be consumed by ESM, thus inviting a branched graph, so this predates Before |
I think once So this Thanks. |
Yes, but at the meantime, before So until
After
The precedence depends on the package author because the conditional exports are applied in insertion order. |
In your example, you are doing something like this:
This is already and will always be unsafe on older versions of Node.js, unrelated to
This is unrelated to dynamic import, by the way. The problematic edge is Let's suppose on v22 we ship
But if Node.js never ships
|
@nodejs/loaders |
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 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 <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 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 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>
What is the problem this feature will solve?
In this MR #51977 it's being proposed to allow CJS to
require(esm)
with the goal of helping people stuck in CJS to use ESM only modules, somehow conflicting with the plethora of dual modules already published and maintained by authors (and I am one of them).To understand the issue there's no better way than a concrete use case.
Where we are now ...
A project using module a as dependency, where a is published as dual module, can be consumed from pure CJS, where by pure I mean no dynamic
import(a)
in the mix, as that usually undesired or not common in CJS land due lack of TLA, as well as pure ESM.module a
module b
In this CJS scenario the module a will always provide the same random number once, no matter how many modules require it.
Now, because it was not possible before to synchronously import module c, the module b has no hazards in a CJS only environment.
Now enters module c as
./c.mjs
Where we're potentially going ...
edit test the use case if you want
In a scenario where the flag lands "unflagged" and the CJS is still the default, developers will carelessly believe they can finally
require(esm)
without thinking twice about possible consequences ... if no error is encountered due TLA in the required ESM module, they think they're good!module b after the flag
Conclusion
It is true that hazards related to dual modules where already possible before but there was no easy way to synchronously require ESM modules so that either they chose dual modules, they used a tool able to normalize everything as CJS or ESM, but any re-published package as CJS would've avoided or inlined somehow ESM modules with TLA.
In short, if the default
require
, when it comes to dual modules, still prefers CJS once this flag lands unflagged, we will see dual module authors blamed for issues that were not so easy to bring in before with potentially catastrophic results beyond the dummyMath.random()
use case: databases, cache, file system, workers, you name it ... bootstrapping with ease dual modules will likely create more damage than solve instead anything it's aiming to solve and currently published dual modules cannot just stop providing their CJS counterpart until all users and dependents modules are capable of requiring ESM out of the box.What is the feature you are proposing to solve the problem?
Before proposing anything I don't understand how this experimental flag is being considered as shippable in its current state (broken ESM require if TLA is used behind, increased dual module hazards surface) instead of fixing at the root level the issue by:
Back to this flag though, I would like to propose at least the following solutions:
require(anything)
, once this flag lands unflagged, is to return the ESM version of the module, if such module is a dual module. This would solve the presented use case / issue because the module b thatrequire("a")
will already have the ESM version so that once module b finally can alsorequire("c")
from the ESM only world, nothing will break and no hazard will be presentrequire
a module from CJS so that dual module authors can explicitly indicate in theirpackage.json
that the preferred way torequire
that module is through its ESM code and not its CJS artifact (or vice-versa whenever that's the case). In this case I believe having the"type": "module"
in thepackage.json
of a SHOULD already be enough to tell the newrequire(esm)
ability that even whenrequire(cjs)
was used, and that module is dual module, the returned module is actually the ESM one.The latter point was breaking before so it should be a no-brainer to consider while the former suggestion might break with
default
exports that were not expected before but I am hear to discuss possible solutions there too or propose we give developers time to test and adjust their code, after all they need to do so anyway the moment they finallyrequire(ESM)
.Having ESM as preferred way to any require will eventually convince developers to publish ESM only modules so that the dual module story can fade away in time, but if that's not the case I see a catch 22 like situation where authors can't stop publishing dual modules and users can't stop worrying about possible hazards introduced by the new feature.
What alternatives have you considered?
As already mentioned, I would rather bring in TLA in CJS behind a flag instead and call it a day, without mixing a tad too much the
require
ability of the legacy, non standard, module system that CJS is. This would be the easiest migration for everyone caring to migrate and it will still grant pure CJS software to work without worrying about hazards in the graph.Thank you.
After Thoughts
It's OK to explore solutions to the problem (we're doing that already) but if NodeJS lands these kind of changes it better be sure that the entirety of the tooling based projects out there also get the memo and implement the right thing or we'll create a new kind of hell to deal with, explain, and resolve ... at least until everything is ESM only and CJS can be just a memory to find in outdated books.
The text was updated successfully, but these errors were encountered: