Skip to content
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

Merged
merged 8 commits into from
Jan 2, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 28, 2021

Refs: #41333

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Dec 28, 2021
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.
Copy link
Member

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)

Copy link
Contributor Author

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?

Copy link
Member

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 :-)

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 an ERR_REQUIRE_ESM error. All other files are parsed as CommonJS scripts.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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

doc/api/modules.md Outdated Show resolved Hide resolved
Co-authored-by: Michaël Zasso <[email protected]>
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a 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.

@ljharb
Copy link
Member

ljharb commented Dec 29, 2021

Why was that decision made? It seems like a reasonable thing to add, if we’re expecting people to rename files to .cjs

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Dec 29, 2021

@Trott im saying they shouldn’t be forced to add the extension when they don’t for any other CJS non-exports entry point.

@GeoffreyBooth
Copy link
Member

When .cjs was introduced I asked if we should add it to the list of automatically appended extensions. I was told that doing so would cause more carbon emissions to be emitted (via additional data center processing) than any other action I would take in my life. I hope that’s hyperbole, but the point was that adding it would be a substantial performance hit. Even if we put it last, so the search order went .js, .json, .node, .cjs, it would still be before ./folder/index.js, and so every require('./folder') reference in anyone’s code would entail an extra filesystem lookup before finding ./folder/index.js, which would be a huge performance regression in Node. Even putting .cjs and ./folder/index.cjs entirely last, out of order relative to the other lookups, would still entail a performance regression for cases where code is using require to check for the nonexistence of a module.

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 .cjs and that requiring it should include the extension, the docs should say so. Likewise if it’s not already documented in the CommonJS docs, we should note that require of .mjs is unsupported and that await import() should be used instead.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 29, 2021

@GeoffreyBooth the added extensions are covered in the previous paragraph:

node/doc/api/modules.md

Lines 376 to 378 in ea977fc

If the exact filename is not found, then Node.js will attempt to load the
required filename with the added extensions: `.js`, `.json`, and finally
`.node`.

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.

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 .cjs?

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 29, 2021

Likewise if it’s not already documented in the CommonJS docs, we should note that require of .mjs is unsupported and that await import() should be used instead.

node/doc/api/modules.md

Lines 134 to 139 in ea977fc

## The `.mjs` extension
It is not possible to `require()` files that have the `.mjs` extension.
Attempting to do so will throw [an error][]. The `.mjs` extension is
reserved for [ECMAScript Modules][] which cannot be loaded via `require()`.
See [ECMAScript Modules][] for more details.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 29, 2021
@GeoffreyBooth
Copy link
Member

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 .cjs?

Personally I think we should call out .cjs explicitly. It doesn’t necessarily need to be here, if you think there’s a better place to mention it, but I think there should be some mention of .cjs in the CommonJS docs and not only in the ESM docs.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 30, 2021

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 .cjs?

Personally I think we should call out .cjs explicitly. It doesn’t necessarily need to be here, if you think there’s a better place to mention it, but I think there should be some mention of .cjs in the CommonJS docs and not only in the ESM docs.

I don’t think ESM docs mention .cjs, but they do mention .mjs

@GeoffreyBooth
Copy link
Member

I don’t think ESM docs mention .cjs, but they do mention .mjs

It’s most prominently mentioned here, I think: https://nodejs.org/api/packages.html#determining-module-system

Regardless, I think .cjs deserves a mention somewhere in the CommonJS docs too, not just in the Packages or ES Modules docs.

@aduh95 aduh95 requested a review from GeoffreyBooth December 30, 2021 14:59
@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 30, 2021
Comment on lines 70 to 75
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

node/doc/api/esm.md

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 Show resolved Hide resolved
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 1, 2022
<!-- type=misc -->

Node.js treats JavaScript code as CommonJS modules by default.
Authors can tell Node.js to treat JavaScript code as CommonJS modules
Copy link
Member

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?

Copy link
Contributor Author

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.

@aduh95 aduh95 merged commit f1658bd into nodejs:master Jan 2, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 2, 2022

Landed in f1658bd

@aduh95 aduh95 deleted the require-non-js branch January 2, 2022 17:20
aduh95 added a commit to aduh95/node that referenced this pull request Jan 2, 2022
targos pushed a commit that referenced this pull request Jan 14, 2022
Refs: #41333

PR-URL: #41345
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
aduh95 added a commit that referenced this pull request Jan 17, 2022
Refs: #41345 (comment)

PR-URL: #41383
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
Refs: nodejs#41345 (comment)

PR-URL: nodejs#41383
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
Refs: #41345 (comment)

PR-URL: #41383
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Refs: #41333

PR-URL: #41345
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Refs: nodejs#41345 (comment)

PR-URL: nodejs#41383
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Refs: #41333

PR-URL: #41345
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
danielleadams pushed a commit that referenced this pull request Feb 26, 2022
Refs: #41345 (comment)

PR-URL: #41383
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants