-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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 require
behavior with non .js
extensions
#41345
Conversation
Review requested:
|
doc/api/modules.md
Outdated
modules loaded with `process.dlopen()`. | ||
`.json` files are parsed as JSON text files, `.node` files are interpreted as | ||
compiled addon modules loaded with `process.dlopen()`. Files using an unknown | ||
extension (or no extension at all) are parsed as JavaScript text files. |
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.
“JavaScript text files” doesn’t tell me whether it’s as a Script/CJS, or as ESM, since JavaScript text files have two parse goals. (i know the answer - that it’s CJS - but it’d be good if this sentence was unambiguous)
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 the wording here is carefully chosen so it applies to both CJS and ESM. Any non-.json
-nor-.node
extension is treated as JS text file, which may be either a module (in which case the require call will throw) or a script. I'm not a fan of ambiguity myself, but I don't think here is the place to list the rules to determine if a JS text file is parsed as script or module. wdyt?
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 if ambiguity is the intention, your PR as-is indeed captures the ambiguity correctly :-)
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 CommonJS loader does parse JS as CommonJS scripts always though, so I think @ljharb's seeking a clarification does make sense - there isn't ambiguity in play.
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.
Calling require
on an ES module doesn't make it CJS, it throws a ERR_REQUIRE_ESM
. Replacing JavaScript text files
with CommonJS modules
would not be an improvement imho. Do you have a suggestion that removes the ambiguity?
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.
Yes you're right, .mjs
and .js
in "type": "module"
are excluded by this error, which does form an ambiguity of sorts.
Perhaps then explain exactly that, and that will solidify the ESM distinction:
.mjs
or.js
files within a"type": "module"
package boundary throw anERR_REQUIRE_ESM
error. All other files are parsed as CommonJS scripts.
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 if we link to the ad-hoc section in packages.md
?
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.
Except for json and .node and wasm files.
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('./file.wasm')
loads file.wasm
as CJS:
mkdir repro
cd repro
echo 'console.log(this !== undefined)' > file.wasm
echo 'require("./file.wasm")' | node
Co-authored-by: Michaël Zasso <targos@protonmail.com>
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.
Something that we should probably document if it isn’t covered already is that while .cjs
is understood by Node, it’s not treated like .js
in CommonJS where require('./file')
could import file.cjs
. Or put another way, that .js
, .json
and .node
(and ./folder/index.{js,json,node}
) are the only extensions that Node “searches for”/adds automatically. But since .cjs
is documented elsewhere as an extension that Node supports, it’s worth mentioning it explicitly in any discussion of which extensions are searched for. I feel like an example along the lines of “trying to import file.cjs
in CommonJS? Use require('./file.cjs')
” would be good to include.
Why was that decision made? It seems like a reasonable thing to add, if we’re expecting people to rename files to .cjs |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Trott im saying they shouldn’t be forced to add the extension when they don’t for any other CJS non-exports entry point. |
When So I don’t think that there would be much appetite for changing the current behavior. (If anything, there’s probably more support for deprecating extension searching entirely, if it could be done without breaking the world.) So that leaves us with trying to doing the best we can with documentation and error messages to try to explain Node’s algorithm. I haven’t reviewed the CommonJS docs lately, but if they don’t mention |
@GeoffreyBooth the added extensions are covered in the previous paragraph: Lines 376 to 378 in ea977fc
Do you think this point is already covered or you think we should still explicitly states that there won't be any extension searching for |
Lines 134 to 139 in ea977fc
|
Personally I think we should call out |
I don’t think ESM docs mention |
It’s most prominently mentioned here, I think: https://nodejs.org/api/packages.html#determining-module-system Regardless, I think |
doc/api/modules.md
Outdated
Node.js treats JavaScript code as CommonJS modules by default. | ||
Authors can tell Node.js to treat JavaScript code as CommonJS modules | ||
via the `.cjs` file extension, the `package.json` [`"type"`][] field, or the | ||
`--input-type` flag. See | ||
[Determining module system](packages.md#determining-module-system) for more | ||
details. |
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.
Node.js treats JavaScript code as CommonJS modules by default. | |
Authors can tell Node.js to treat JavaScript code as CommonJS modules | |
via the `.cjs` file extension, the `package.json` [`"type"`][] field, or the | |
`--input-type` flag. See | |
[Determining module system](packages.md#determining-module-system) for more | |
details. | |
Node.js has two module systems: CommonJS and ES modules. | |
Node.js treats JavaScript code as CommonJS modules by default. | |
Within an ES module context, authors can tell Node.js to treat | |
JavaScript code as CommonJS modules via the `.cjs` file extension, | |
the `package.json` [`"type"`][] field, or the `--input-type` flag. See | |
[Determining module system](packages.md#determining-module-system) for more | |
details. |
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.
Everything is an es module context. Type module doesn’t create an “es module context”, it just changes the default parse goal for .js files.
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 guess, put another way - what exactly is an “ES module context”? Within a filesystem, you can use extensions with or without the type module flag; in a command line, you can use the input-type flag.
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 not sure I understand what would an ES module context mean either. Also, FWIW I've copied this section from esm.md
, so if we want to change its wording maybe it's best to make a separate PR for that? I'm tempted to remove this change from this PR so it can be discussed separately. wdyt?
Lines 97 to 102 in 2bc1d92
Node.js treats JavaScript code as CommonJS modules by default. | |
Authors can tell Node.js to treat JavaScript code as ECMAScript modules | |
via the `.mjs` file extension, the `package.json` [`"type"`][] field, or the | |
`--input-type` flag. See | |
[Modules: Packages](packages.md#determining-module-system) for more | |
details. |
doc/api/modules.md
Outdated
<!-- type=misc --> | ||
|
||
Node.js treats JavaScript code as CommonJS modules by default. | ||
Authors can tell Node.js to treat JavaScript code as CommonJS modules |
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 weird to say “it’s the default, and here’s how you can make it apply”.
For file extensions, .cjs is always CJS, .mjs is always ESM, .js is CJS by default (but type module can make this be ESM), and anything else is either .json, .node, .wasm, or “unknown” (which is treated as CJS). Piped input is CJS by default, but can be made ESM with the input-type flag.
Can we say something basically exactly like that?
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'll remove this section from this PR and move it to a separate PR so it can be discussed separately if that's ok.
Landed in f1658bd |
Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Refs: nodejs#41345 (comment) PR-URL: nodejs#41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Refs: nodejs#41333 PR-URL: nodejs#41345 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>
Refs: nodejs#41345 (comment) PR-URL: nodejs#41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Refs: #41333