Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
doc: clarify module system selection #41383
doc: clarify module system selection #41383
Changes from 6 commits
1b1159a
dd7140f
97515fd
c19d161
ce3edd0
be6f791
e9c0b15
4c697b7
2cea6a8
21b45bf
93d6a23
fe05b72
5de6698
b08ec87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think you should explicitly refer to
--input-type
. I know that--loader
also currently makes the entry point esm, but that's a bug (cf #33226, cc @cspotcode).Formalizing this behaviour on
--loader
is problematic because it means whoever sets must also be sure that the entry point is esm. This isn't always the case:NODE_OPTIONS
is typically set long beforenode
is ever called, and by processes which aren't necessarily aware of what the entry point will be (or even if there will only be one of them).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.
To save readers some time, this is the comment where we established that it is, indeed, a bug: #33226 (comment)
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 PR is trying to describe the current behavior of Node.js, not its final or ideal state. I don't call out which CLI flags do trigger this behavior precisely because of that discussion you linked to.
--experimental-specifier-resolution=node
also triggers this behavior.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.
The nature of the fix isn't clear yet so idk if this PR needs to fix it right now. I wouldn't want to block this PR based upon #33226 .
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 only meant that documenting the interaction between
--loader
and entry points (even if--loader
isn't explicitly referred to in this paragraph) may have a slight risk of suggesting to readers that it can be relied upon and that it'd be a regression to fix it. While loaders are experimental, I noticed people often expect experimental-induced breaking changes to be very visible in their API (like the hook refactoring) rather than subtle under-the-hood behavioural changes.Note that this wasn't a blocker by any means, just a suggestion.
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 just don’t think the docs should have any references to “the CommonJS modules loader” or the “ECMAScript modules loader”—no average user knows what those are. There’s just Node, and how it interprets source code.
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.
OK but using the ES module loader has more effect than on simply JS code (it no longer accepts
.node
and.json
file as entry point, it no longer treats extensionless / unknown extension as JS files), which was the nuance I was trying to communicate here.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.
Let's add those other implications here, then. Most people reading these docs wouldn't know about those nuances just because the loader is mentioned.
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've tried to address that in ce3edd0, PTAL.
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.
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 don't think that's true, if the specifier points to an existing directory, then an exact match is found – we have a Folders as modules section that explains it. Otherwise, we would have to specify it first tries to load the
"main"
field if the directory contains apackage.json
file, we might as well just link to that section (EDIT: which I've done in the very next item, I think we should leave it as is)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.
Then maybe mention the folder resolution without spelling it out?
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.
Does that work?node/doc/api/packages.md
Lines 97 to 99 in 93d6a23
EDIT: no you're right, we should document the order here.
node/doc/api/packages.md
Lines 97 to 100 in fe05b72
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.
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 don't know if you noticed, but I tried to list the same features in both lists, so user can compare the difference between CJS and ESM loaders. If we add an item here, can we add it to the ESM list as well?
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.
You could combine the two
require
-related ones, maybe?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.
It also allows ES module to load a CJS module using
require
function. I'm not sure this information is useful tbh. Is it correct that the information you are trying to convey is that all JS files are treated as CJS? If so sure I can add that.I don't see how this is related to the CJS loader.
import()
is exposed in CJS modules, it is explicitly called out is several places, but I think it would add too much confusion to listimport()
as a CJS loader feature.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 went with
node/doc/api/packages.md
Lines 103 to 107 in 21b45bf
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.
What I don't "like" with this last version is it gives the impression that if an ESM file do not use any ESM specific syntax, CJS loader would (try to) parse it as CJS. Can we have a version without using
tries to
?Do we really need to document here that
ERR_REQUIRE_ESM
is thrown? It's already documented inmodules.md
, I'd prefer if we leave it out to simplify that list item.ESM loader can load more than just ESM, so it may not be obvious that CJS only loads CJS?
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.
Can it though? Doesn’t it just call the CommonJS loader to handle cases of
import
of CJS? Just like how the CommonJS loader calls the ESM loader to handle cases ofimport()
of ESM?We can leave out the error message, sure. But then I don’t think there’s anything left of this bullet point, and so we should just remove it. Both loaders use the other to handle the other system’s modules. The only thing worth mentioning is how
import
/import()
can handle either module system whilerequire
only supports ESM. That’s a really important point that we should make if we’re not doing so already, and is much more valuable to users than a description of how the internals of the loaders work.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.
(
require
only supports CJS)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.
IMHO there's a clear difference: with
Module._load
there is no way to load a file as ESM; withesmLoader.load
, you can load a file as CJS. That's what I'm talking about, maybe it's not an information relevant for end users, but I believe it is for loader hooks authors.require
andimport
are user facing features, and are already documented elsewhere, it's easier if we definerequire => CJS loader
andimport => ESM loader
(and it's mostly true AFAIK), and I think it would be detrimental to imply thatimport
andrequire
behave differently depending on the parse goal of the file.I'm going to land as is, hopefully we can improve this part of the docs in future PRs.
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.
But can it really? If you
import
a CommonJS file, isn’t it just passing that off to the CommonJS loader for loading? So in that sense, each loader only handles its domain.CommonJS modules load more than just CommonJS, and the inverse for ES modules, and that’s what matters to end users. Stating that the loader can only handle CommonJS just adds confusion.
I think that matters to users is that
require
can only take CommonJS, whileimport
/import()
can take either. And thatimport
doesn’t exist in CommonJS, butimport()
does.All this other stuff about what the loaders can do is unnecessary detail that can confuse people. I understand the desire to keep the whole list focused on the CommonJS loader, which is why I’m thinking it’s best to just leave out this detail unless you want to make this bullet about
require
vsimport
rather than about the loader itself.