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.

Sign up for GitHub

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

Jump to bottom

doc: clarify module system selection #41383

Merged
merged 14 commits into from
Jan 17, 2022
Merged

doc: clarify module system selection #41383

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1b1159a
doc: clarify module system selection
aduh95 Jan 2, 2022
dd7140f
Address review comments
aduh95 Jan 5, 2022
97515fd
fixup!
aduh95 Jan 7, 2022
c19d161
fixup!
aduh95 Jan 7, 2022
ce3edd0
fixup!
aduh95 Jan 8, 2022
be6f791
fixup!
aduh95 Jan 10, 2022
e9c0b15
fixup!
aduh95 Jan 11, 2022
4c697b7
Apply suggestions from code review
aduh95 Jan 13, 2022
2cea6a8
fixup!
aduh95 Jan 13, 2022
21b45bf
fixup!
aduh95 Jan 13, 2022
93d6a23
fixup!
aduh95 Jan 14, 2022
fe05b72
fixup!
aduh95 Jan 14, 2022
5de6698
fixup!
aduh95 Jan 16, 2022
b08ec87
fixup!
aduh95 Jan 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions doc/api/esm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,12 @@ provides interoperability between them and its original module format,

<!-- type=misc -->

Node.js treats JavaScript code as CommonJS modules by default.
Authors can tell Node.js to treat JavaScript code as ECMAScript modules
Node.js has two module systems: [CommonJS][] modules and ECMAScript modules.

Authors can tell Node.js to use the ECMAScript modules loader
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
Authors can tell Node.js to use the ECMAScript modules loader
Authors can tell Node.js to treat JavaScript code as ECMAScript modules

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.

Copy link
Contributor Author

@aduh95 aduh95 Jan 7, 2022 •
edited
Loading

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.

Copy link
Member

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.

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've tried to address that in ce3edd0, PTAL.

via the `.mjs` file extension, the `package.json` [`"type"`][] field, or the
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
`--input-type` flag. See
[Modules: Packages](packages.md#determining-module-system) for more
details.
[`--input-type`][] flag. Outside of those cases, Node.js will use the CommonJS
module loader. See [Determining module system][] for more details.
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

<!-- Anchors to make sure old links find a target -->

Expand Down Expand Up @@ -1425,6 +1425,7 @@ success!
[CommonJS]: modules.md
[Conditional exports]: packages.md#conditional-exports
[Core modules]: modules.md#core-modules
[Determining module system]: packages.md#determining-module-system
[Dynamic `import()`]: https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Dynamic_Imports
[ECMAScript Top-Level `await` proposal]: https://github.com/tc39/proposal-top-level-await/
[ES Module Integration Proposal for WebAssembly]: https://github.com/webassembly/esm-integration
Expand All @@ -1437,6 +1438,7 @@ success!
[WHATWG JSON modules specification]: https://html.spec.whatwg.org/#creating-a-json-module-script
[`"exports"`]: packages.md#exports
[`"type"`]: packages.md#type
[`--input-type`]: cli.md#--input-typetype
[`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer
[`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
[`TypedArray`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
Expand Down
16 changes: 15 additions & 1 deletion doc/api/modules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,18 @@ module.exports = class Square {
};
```

The module system is implemented in the `require('module')` module.
The CommonJS module system is implemented in the [`module` core module][].

## Enabling

<!-- type=misc -->

Node.js has two module systems: CommonJS modules and [ECMAScript modules][].

Authors can tell Node.js to use the ECMAScript modules loader
via the `.mjs` file extension, the `package.json` [`"type"`][] field, or the
[`--input-type`][] flag. Outside of those cases, Node.js will use the CommonJS
module loader. See [Determining module system][] for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a note again that the ES module loader can be accessed in any context via dynamic import().

aduh95 marked this conversation as resolved.
Show resolved Hide resolved

## Accessing the main module

Expand Down Expand Up @@ -1047,13 +1058,16 @@ This section was moved to
[ECMAScript Modules]: esm.md
[GLOBAL_FOLDERS]: #loading-from-the-global-folders
[`"main"`]: packages.md#main
[`"type"`]: packages.md#type
[`--input-type`]: cli.md#--input-typetype
[`ERR_REQUIRE_ESM`]: errors.md#err_require_esm
[`Error`]: errors.md#class-error
[`__dirname`]: #__dirname
[`__filename`]: #__filename
[`import()`]: https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Dynamic_Imports
[`module.children`]: #modulechildren
[`module.id`]: #moduleid
[`module` core module]: module.md
[`module` object]: #the-module-object
[`package.json`]: packages.md#nodejs-packagejson-field-definitions
[`path.dirname()`]: path.md#pathdirnamepath
Expand Down
34 changes: 26 additions & 8 deletions doc/api/packages.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,13 @@ along with a reference for the [`package.json`][] fields defined by Node.js.
## Determining module system

Node.js will treat the following as [ES modules][] when passed to `node` as the
initial input, or when referenced by `import` statements within ES module code:
initial input, or when referenced by `import` statements or `import()`
expressions:

* Files ending in `.mjs`.
* Files whose name ends in `.mjs`.
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

* Files ending in `.js` when the nearest parent `package.json` file contains a
top-level [`"type"`][] field with a value of `"module"`.
* Files whose name ends in `.js` when the nearest parent `package.json` file
contains a top-level [`"type"`][] field with a value of `"module"`.

* Strings passed in as an argument to `--eval`, or piped to `node` via `STDIN`,
with the flag `--input-type=module`.
Expand All @@ -67,12 +68,12 @@ field, or string input without the flag `--input-type`. This behavior is to
preserve backward compatibility. However, now that Node.js supports both
CommonJS and ES modules, it is best to be explicit whenever possible. Node.js
will treat the following as CommonJS when passed to `node` as the initial input,
or when referenced by `import` statements within ES module code:
or when referenced by `import` statements or `import()` expressions:

* Files ending in `.cjs`.
* Files whose name ends in `.cjs`.

* Files ending in `.js` when the nearest parent `package.json` file contains a
top-level field [`"type"`][] with a value of `"commonjs"`.
* Files whose name ends in `.js` when the nearest parent `package.json` file
contains a top-level field [`"type"`][] with a value of `"commonjs"`.

* Strings passed in as an argument to `--eval` or `--print`, or piped to `node`
via `STDIN`, with the flag `--input-type=commonjs`.
Expand All @@ -83,6 +84,23 @@ future-proof the package in case the default type of Node.js ever changes, and
it will also make things easier for build tools and loaders to determine how the
files in the package should be interpreted.

Node.js will refuse to load the following when passed to `node` as the
initial input and the nearest parent `package.json` file contains a top-level
[`"type"`][] field with a value of `"module"`:

* Files whose name doesn't end in `.js`, `.mjs`, `.cjs`, or `.wasm`.
Copy link
Member

Choose a reason for hiding this comment

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

Rather than saying what Node won’t do, why not say what it will? I think it’s easier to understand the positive case than the negative:

Suggested change
Node.js will refuse to load the following when passed to `node` as the
initial input and the nearest parent `package.json` file contains a top-level
[`"type"`][] field with a value of `"module"`:
* Files whose name doesn't end in `.js`, `.mjs`, `.cjs`, or `.wasm`.
When the nearest parent `package.json` file contains a top-level
[`"type"`][] field with a value of `"module"`, the `node` command will
accept as input only files with `.js`, `.mjs`, or `.cjs` extensions;
and with `.wasm` extensions when `--experimental-wasm-modules` is enabled.

(And please add a link to --experimental-wasm-modules; and is a wasm entry point actually supported?)

Copy link
Member

Choose a reason for hiding this comment

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

The type field has no bearing on what node accepts - it accepts a .js file regardless, and has no effect on the others.

Copy link
Member

Choose a reason for hiding this comment

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

That isn't true, type affects if the ESM module loader is used for the entry point:

node/lib/internal/modules/run_main.js

Line 41 in 2cc7a91

return pkg && pkg.data.type === 'module';
. Also it makes extension-less files fail to load and that should probably be called out.

Copy link
Member

Choose a reason for hiding this comment

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

For the latter, that’s good to know, and should indeed be called out.

For the former, are you saying node foo won’t do any extension lookup on foo.cjs,foo.js, or foo.mjs? If so, i understand why - but that’s not enabling ESM, that’s disabling the standard/default CJS loader, and that should definitely be explicitly called out as well (as a potential hazard/impact of using type module)

Copy link
Member

Choose a reason for hiding this comment

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

under type:"module" the regular ESM semantics are applied against the preresolved filepath argv[1] absolute file: URL; so, no searching occurs. For type:"commonjs" the CJS semantics are applied against the preresolved argv[1] absolute file path.

Copy link
Member

Choose a reason for hiding this comment

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

so it looks like CJS resolver is always hit prior to the the ESM resolver, but things are weird and spaghetti

node/lib/internal/modules/run_main.js

Line 70 in e46c680

const resolvedMain = resolveMainPath(main);
, so it does both, not one or the other when it thinks it is ESM. there is an odd comment about back compat, but I have no memory of a compat issue.

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb the comments in the file that are causing it explicitly state that removing extension searching can be done in the future.

Copy link
Member

Choose a reason for hiding this comment

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

also, this seems like it would break if someone mucked with require.extensions in weird ways and passed it to ESM

Copy link
Member

Choose a reason for hiding this comment

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

The comments were probably from when it was still experimental. Now that it no longer is, it doesn't seem like something that can be removed without a semver-major.

Copy link
Member

Choose a reason for hiding this comment

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

Likely would be semver-major yes.


Copy link
Member

Choose a reason for hiding this comment

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

This is perhaps where we should mention that ESM entry points need to be URLs.

Suggested change
When the initial input is ES module JavaScript, the reference to it must be
given as a URL, not a file path. For example: `node entry.mjs`;
`node ./app/entry.mjs`; `node file:///c:/path/to/app/entry.mjs`.

I feel like others can come up with better wording; my point is just that something along these lines should be part of the docs, either in the ESM or CLI docs or probably both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's.. not true? AFAIK you can only specify a path as CLI entry point, using a URL only works if it happens to coincide with a path.

$ cd /tmp
$ touch file.mjs
$ node file:///tmp/file.mjs
node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module '/private/tmp/file:/file/path'
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Node.js v17.3.0
$ node ./%66ile.mjs
node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module '/private/tmp/%66ile.mjs'
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Node.js v17.3.0

Copy link
Member

Choose a reason for hiding this comment

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

Takes a deep breath the CLI never accepts a filepath or a url. It is always a string specifier resolved using file semantics against the cwd PRIOR to doing any loading stuff

node/lib/internal/bootstrap/pre_execution.js

Line 105 in e46c680

process.argv[1] = path.resolve(process.argv[1]);
. It would resolve the same as a URL or filepath semantically. This causes confusions since it uses file resolution, but that is something that would be too dangerous to change last I talked with others about it (percent encoding problems for example). The increased usage of URL in various places may change that, but even if the CLI was to move to be URL based it likely would need a flag, but it does use URLs when it goes into the ESM Loader.

Copy link
Member

Choose a reason for hiding this comment

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

So in other words, it is true?

This was what tripped me up in the tests PR, where the tests were failing only in Windows and it took me a long time to figure out why.

Copy link
Contributor Author

@aduh95 aduh95 Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It would resolve the same as a URL or filepath semantically.

My understanding is it only accepts a CJS specifier (with the notable difference that if you don't specify ./, it still tries to resolve it as a relative path, but it still does extension searching), because of this Module._findPath+path.resolve call:

node/lib/internal/modules/run_main.js

Line 16 in 2cc7a91

let mainPath = Module._findPath(path.resolve(main), null, true);

At least this discussion shows that this part of the docs definitely needs some clarification, because I'm quite confused right now ^^

This was what tripped me up in the tests PR, where the tests were failing only in Windows and it took me a long time to figure out why.

I think there's a confusion with --experimental-loader that takes a URL and never a path (even if the loader is a .cjs file) – or maybe a more correct way of saying it would be: it accepts any specifier that can be resolved by ESM loader relative to cwd?

Copy link
Member

Choose a reason for hiding this comment

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

--loader differs greatly reading through edge case carved out code. It was always intended to be a URL for forwards compatibility and to avoid some questions about passing in an entire loader via ARGV without a file via data:.

Passing to `node` as the initial input when the nearest parent `package.json`
file contains a top-level [`"type"`][] field with a value of `"commonjs"`, or
when referenced by `require()` calls:

* Files whose name ends in `.node` are interpreted as
compiled addon modules loaded with `process.dlopen()`.

* Files whose name ends in `.json` are parsed as JSON text files.

* Any other files will be treated as a [CommonJS][] module.

aduh95 marked this conversation as resolved.
Show resolved Hide resolved
### `package.json` and file extensions

Within a package, the [`package.json`][] [`"type"`][] field defines how
Expand Down