-
Notifications
You must be signed in to change notification settings - Fork 66
adjusted proposal: ES module "esm": true package.json flag #60
Conversation
/cc @bmeck @nodejs/ctc |
/cc @jdalton @nodejs/collaborators |
/cc @ljharb |
xxx-package-module-property.md
Outdated
|
||
* A package with only a `main` and no `module` property will be loaded as | ||
containing CommonJS modules only. | ||
* A package with only a `module` property and no `main` property will be loaded |
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.
This seems to glance over the case of index.js
. E.g. right now a package with index.js
, no main
but a module
would be loaded as CommonJS, using index.js
as the entry point.
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.
Note that all existing requires continue to work the same as we aren't changing the CommonJS resolver. A package with index.js
no main
but a module
property would use the module
property over the main
property under the ES module resolver yes.
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.
I'm confused; why is this needed given .mjs
? What's the point of "keeping the .js extension"?
File extensions answer only ONE question: "how do I parse the file?". .js
parses as a Script. Something else needs to mean "parse as a Module".
Separately, I think that the very existence of "module" elsewhere in the ecosystem should be a dealbreaker for its use in node core - in other words, the only way a new package.json property should be accepted is if ZERO packages are published with it.
xxx-package-module-property.md
Outdated
In the case where a package publicly exposes sub-modules, it would need | ||
to document that the CommonJS and ES Module sources are at different paths - | ||
`import {x} from 'pkgName/submodule.js'` and | ||
`import {x} from 'pkgName/cjs/submodule.js'`. Or simply a `.js` and |
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.
Given that it looks like we'll use import './x.mjs';
and import './x.js'
so separate relative imports into module vs. CJS, is there a reason why we'd want to allow pkgName/submodule.js
to be an ES module? This seems inconsistent / surprising. And if we don't, we wouldn't need the "module": true
workaround below. import 'pkg/sub.mjs'
and import 'pkg/sub.js'
would have well-defined semantics.
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.
This proposal aims to provide a path for allowing .js
for es modules. It is fully orthogonal to "use module"
and .mjs
, so the workflows you discuss still remain completely viable.
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.
(actually not just orthogonal, but this proposal supports "use module"
and .mjs
as expected)
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.
This... seems like a pretty big deal that import {x} from 'pkgName/submodule'
and require('pkgName/submodule')
no longer work without modification for mixed packages?
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.
I should probably switch the example around to pkgName/es/submodule
rather and reword a bit here...
The mixed package use case is possible but by no means necessary. Libraries looking to support stable NodeJS versions can continue to ship CommonJS. Libraries looking to support newer versions of Node only can choose to just ship ES modules. For the mixed case, libraries can likely document the CommonJS version as the main API (pkg/x
) while informing knowledgeable users about the ES modules distribution at pkg/es/x
for users who can get build benefits etc from this.
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.
I would love to see module.root to be added as well. This would allow hiding implementation details for modules and enable painless migartion/deprecation.
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.
@chyzwar as stated at the top of this proposal, the EP is not attempting to garner support for modules.root
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.
I know It is wishful thinking. Let first get ESM before v10. Otherwise, at my current job, I would need to wait another 3 years before it gets approved.
I thought we had pretty much decided against relying on/requiring package.json for ES modules? |
Not sure it's summed up in a tidy fashion anywhere, though. Best I've found is to go to #3 and use my browser's "find" feature to search for terms like |
https://github.com/nodejs/node-eps/pull/3/files#diff-0355b3cc08860f0ddbc8338ae885599aR141 summarizes "other proposals", but unfortunately doesn't explain the reasons they weren't suitable. |
To explain the need here, there is a very definite desire from the JS community to continue to be able to use the In terms of this being a breaking change - note that the ES resolver is a new resolver and new behaviour (under this spec). Nothing about the existing CommonJS resolver is being changed under this proposal. |
And thanks all for the feedback so far :) |
This looks like it doesn't interfere with any current plans / tribulations for Node ( We should review how existing tooling works compared to this PR and talk about the edge cases mentioned with a vantage point of if it causes conflicts with other plans. I am fairly neutral to this personally since I see it as an additive, so I won't be giving a +/- unless it regards compatibility. Even if this is not merged into Node's core I would like to see tools at least standardizing what they do. I feel this is as good a place to talk about such standardization as any since compatibility with Node is a priority for such tools. I have some personal life things going on and this PR is not my main focus for a few weeks, but I don't see any glaring incompatibilities that cause conflicts. It does introduce/require package boundaries, but this was already hinted at in the ESM stuff as it would be required to prevent fall through. If nothing else, agreeing upon package boundaries would be a good thing. For now, and action item would be to get a compatibility table of what |
I've noticed this as well, but is there a justification for this aside from "a lot of people seem to want it"? As someone who previously wanted to keep using |
@not-an-aardvark this does not attempt to replace |
I realize that, but I'm a bit averse to adding several different ways to indicate that a file is a module, because I think it would end up being very confusing. (I see how my previous comment was unclear, so I've edited it to clarify.) |
@guybedford could you provide a link to the specification reference to the One advantage to At the same time, having too many ways of doing the same thing is definitely an anti-pattern we should be aware of, and something that can easily confuse / overwhelm new comers. |
As in the In Defense of JS proposal, the goal ultimately is to have @MylesBorins thanks for the feedback. The module field handling is in the path lookup process at https://github.com/nodejs/node-eps/pull/60/files?diff=unified#diff-b193302c6b58f3c92b9098094d833b64R178, which is given a file system path, and does the file extension / main resolution for it. I've removed this from the CTC meeting agenda item for today, as it seems like I may have got ahead of the correct process here. Going forward I'd really appreciate any guidance from CTC for how best to discuss this at a high level face to face, so we can start to have that more nuanced discussion over the different levels at which this kind of approach applies. Thanks for all consideration so far. |
I think Moreover, this proposal creates packages built with common.js and packages built using the new ES6 modules. This makes using both within the same application very hard or impossible. I am in favor of the |
@mcollina As I understand, most projects will eventually use modules for all source files, so we will not need fine-grained control over how a given file is parsed. |
@hax that's the hope, surely - but that fine-grained control is utterly essential for even the possibility of getting there. |
What does this solve that I am not very much in favor of adding multiple ways to do the same thing, that will only end in confusion. |
@Fishrock123 great point and it's important to be clear what we're actually solving certainly. And yes it's a human problem mostly. I know this has been discussed much, and the CTC is far more accomplished at dealing with these human problems, but here is my take on things. Currently in NodeJS I can write a The thing here is that most users already write ES modules as So to now expect those same developers to rewrite all these files to Yes there will be many small changes from the handling of interop, named exports, core modules etc in the process of transitioning to actual ES modules from these codebases that will cause problems and frustrations, but we should make the greatest effort, so far as those efforts are not unnecessarily complex certainly, to provide what we can to make users' lives easier in this process - I hope we can agree that ultimately the goal of such a project as NodeJS is to enable developers, not to restrict them. So in the name of allowing those with this emotional attachment to dot js not have to surrender their ".js" happiness, or having to instead begrudgingly write I don't think there would be an immediate backlash at all for Node to release modules with just ".mjs". But as the bulk of users actually start writing and migrating their primary codebases to ES modules in this way, there becomes a desire for alternatives if that frustration is too much. @jdalton has already made it clear that he is working on a loader for NodeJS to retain a ".js" extension approach and will be sharing this approach widely. It only seems productive to have a plan for handling such possible outcomes. I'm not saying we should ship this approach today by any means, I'm just saying that having an approach and an active, relevant and current approach that is compatible with the way modules work in Node through the current PR implementation while being compatible with ".mjs" and "use module", is important to be able to say to those who question ".mjs" that there is an alternative, that it has been thought of, and that if they are interested there is a link to this proposal (or another), where they can engage, participate and help it become popular enough to gain adoption. If there is no interest, certainly lets drop it in future. But having users just see 5 proposals all abandoned would only lead to more frustration, so I'm just trying to provide some path here that people might get on board with, preferably without adding to that list! |
One very small (but important, I think) detail is that they write ES modules using what are basically CommonJS semantics, especially around loading. E.g. they assume that code gets executed without event loop ticks, they assume that all the wild path resolution rules used by node, including magical file extensions, "just work the same way", they make use of CommonJS-y identifiers like Taking that code and assuming it would also work in a true ES6 module environment is a rather big assumption. At least from some initial exploration in our own code base, it doesn't really work out. So splitting those ""ES modules"" that end with |
We should be removing extraneous checks that degrade performance, Node already has too many of them, not adding more. Especially when the only value added is not wanting to use a different file extension. Much better, I think, to add some sort of |
@matthewp it's important to keep the statting down certainly, but if that is your concern, do you inline extensions into your module requires (this would save two stats per module load!)? We're talking of the order of two stats here anyway, so it is the same difference of one inlined extension over a codebase of hundreds of modules! Important to consider these numbers in perspective. Certainly any work here should be carefully benchmarked. If you believe strongly in an alternative approach, please do propose something though. |
@thejameskyle the poll results show ~55% for "esm" and ~37% for "modules" and ~7% for neither (over 774 votes, with 1% in these numbers lost to rounding). Would this be enough for you as some consensus here? |
Well, the results are inherently scewed towards people that already know what "esm" means... which isn't the crowd of people I'm concerned about. Although I'm honestly surprised people who do know what "esm" means actually want that as the property. |
@guybedford Yes, I do include the extension in my require calls. Whether someone does that or not is up to them and only affects their code. The troublesome thing about this proposal is that it slows down all uses of Node, whether this feature is being taken advantage of or not. A hello world Node script will now traverse directories searching for a package.json, parse it if it finds one, and then do what you wanted it to do. |
@matthewp yes Node will resolve modules, including loading package.json files for node_modules packages before executing your code, I'm not sure what about that is surprising. |
I think you're misunderstanding me, if the program is: hello.js console.log("Hello world!"); > node hello.js My understanding is that, today, this will open hello.js (and only hello.js) and execute it as a CommonJS module. And under your proposal it will search for a package.json, parse the package.json (if it finds one), and then open hello.js. Is this inaccurate in either case? Node doesn't, today, start resolving modules until there is a require() call, right? |
What I don't understand is why you're focusing on a simple case, when the majority of NodeJS applications have dependencies. I'm simply trying to shift the argument back to the important case of a larger application with dependencies, where dependency resolution is the primary resolver overhead, and noting that your case is completely negligible in comparison. |
I'm focused on this case because Node's resolver is already a problem and I don't want it to become worse. It's important to remember that Node.js is used in a variety of contexts and not just for large server applications. I've personally been forced, in the past, to bundle a CLI program because the resolver was too slow. Executing scripts in a loop is a real thing! As far as I'm aware, at least today all slow paths are opt-in (module identifiers that point to directories, using |
@matthewp yes as the proposal stands that extra package.json load check would not be opt-out. Thinking about this, an --esm flag could possibly be added here to override treating the entry point as being at an ES modules directory boundary, overriding the package.json check. Would such an addition work for you here? |
I'm pretty uneasy with any unwanted side-effect file opening. I would certainly hope that I'll refrain from commenting further to avoid being too negative about this idea. What I really hope that the Node maintainers consider is that these sorts of sugar proposals, while nice in some ways, can wait until after the initial release of ES modules. I hope that it is initially released only supporting |
FWIW Perf isn't an issue since the lookup is done once per directory not per module in the directory. I test with |
was there any resolution to using |
@jdalton yes this proposal is similarly done once per directory and cached for the lifecycle of the application. Glad to hear it hasn't been a perf issue for your users. |
This allows ".js" files to be loaded as es modules whenever the parent package.json file contains "esm": true. Original proposal and discussion at nodejs/node-eps/pull/60
This allows ".js" files to be loaded as es modules whenever the parent package.json file contains "mode": "esm". Original proposal and discussion at nodejs/node-eps/pull/60
I imagine this conversation, if still ongoing, is happening elsewhere. Please comment or re-open though if closing this isn't the right move. |
For reference, this topic seems no longer discussed but most of the discussions around modules moved to the nodejs/modules repo. |
Update: This proposal has been updated to reflect the direction I believe to be the simplest and best for the ecosystem - an
"esm": true
flag in the package.json file.The reason for this change from
module
is due to the interactions ofmodule
with other conditional main systems resulting in much unwanted complexity. For examplebrowserModule
was the natural next step here, which I don't think is a road that would be good for the ecosystem at all.(added with comment at #60 (comment))
This spec provides a package.json"module"
property and resolver implementation approach, to allow distinguishing ES modules in NodeJS. Unlike the In Defense of Dot JS proposal, it excludes supporting the"modules"
and"modules.root"
properties, while providing alternatives to the workflows these were designed for.I've found in my own tooling work that having an active resolver spec would help a lot to ensure convergence towards where NodeJS will ultimately be heading. Webpack and Rollup both already implement this package.json"module"
property, but the edge cases are not completely clear. If we have a formal active spec for"module"
, hopefully we can start to get everyone on board with a future-facing solution here.