doc: clarify `require` behavior with non `.js` extensions by aduh95 · Pull Request #41345 · nodejs/node · GitHub

Refs: https://github.com/nodejs/node/discussions/41333

Review requested:

@nodejs/modules

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

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?

I think if ambiguity is the intention, your PR as-is indeed captures the ambiguity correctly :-)

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.

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?

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.

What if we link to the ad-hoc section in packages.md?

Except for json and .node and wasm files.

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>

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

@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 .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.

@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?

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.

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.

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

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.

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.

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.

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.

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.

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?

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: nodejs#41345 (comment)

Refs: https://github.com/nodejs/node/discussions/41333 PR-URL: #41345 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Refs: https://github.com/nodejs/node/discussions/41333 PR-URL: #41345 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

doc: clarify require behavior with non .js extensions

79a6c9a

Refs: https://github.com/nodejs/node/discussions/41333

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

ljharb reviewed Dec 28, 2021

View reviewed changes

targos reviewed Dec 28, 2021

View reviewed changes

Comment thread doc/api/modules.md Outdated

Update doc/api/modules.md

9d02904

Co-authored-by: Michaël Zasso <targos@protonmail.com>

GeoffreyBooth approved these changes Dec 29, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, 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

parse goal

761197c

guybedford approved these changes Dec 30, 2021

View reviewed changes

.cjs and .mjs in CJS

046e102

aduh95 requested a review from GeoffreyBooth December 30, 2021 14:59

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 30, 2021

GeoffreyBooth reviewed Dec 31, 2021

View reviewed changes

.cjs files always need their extension to be included

5ed7e1c

GeoffreyBooth approved these changes Jan 1, 2022

View reviewed changes

clean up links

40b46cb

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 1, 2022

ljharb reviewed Jan 1, 2022

View reviewed changes

aduh95 added 2 commits January 2, 2022 15:59

Remove enbling section as it doesn't have consensus

7639c2f

remove repetition

9407cd5

aduh95 merged commit f1658bd into nodejs:master Jan 2, 2022

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

doc: clarify module system selection

1b1159a

Refs: nodejs#41345 (comment)

aduh95 mentioned this pull request Jan 2, 2022

doc: clarify module system selection #41383

Merged

targos mentioned this pull request Jan 16, 2022

v17.4.0 release proposal #41557

Merged

aduh95 added a commit that referenced this pull request Jan 17, 2022

doc: clarify module system selection

8732591

Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

BethGriggs pushed a commit that referenced this pull request Jan 25, 2022

doc: clarify module system selection

8429216

Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

danielleadams mentioned this pull request Feb 1, 2022

v16.14.0 proposal #41804

Merged

danielleadams pushed a commit that referenced this pull request Feb 26, 2022

doc: clarify module system selection

a6758d1

Refs: #41345 (comment) PR-URL: #41383 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

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

	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: clarify `require` behavior with non `.js` extensions by aduh95 · Pull Request #41345 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: clarify require behavior with non .js extensions - #41345

doc: clarify require behavior with non .js extensions#41345aduh95 merged 8 commits intonodejs:masternodejs/node:masterfrom aduh95:require-non-jsaduh95/node:require-non-jsCopy head branch name to clipboard

Conversation

aduh95 commented Dec 28, 2021

Uh oh!

nodejs-github-bot commented Dec 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GeoffreyBooth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb commented Dec 29, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

ljharb commented Dec 29, 2021

Uh oh!

GeoffreyBooth commented Dec 29, 2021

Uh oh!

aduh95 commented Dec 29, 2021

Uh oh!

aduh95 commented Dec 29, 2021

Uh oh!

GeoffreyBooth commented Dec 29, 2021

Uh oh!

aduh95 commented Dec 30, 2021

Uh oh!

GeoffreyBooth commented Dec 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Jan 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

doc: clarify require behavior with non .js extensions#41345
aduh95 merged 8 commits into
nodejs:masterfrom
aduh95:require-non-js

GeoffreyBooth left a comment •
edited
Loading