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 module system selection #41383

Merged
merged 14 commits into from Jan 17, 2022
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 2, 2022

Refs: #41345 (comment)

//cc @nodejs/modules

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 2, 2022
@aduh95 aduh95 added the module Issues and PRs related to the module subsystem. label Jan 2, 2022
doc/api/esm.md Show resolved Hide resolved
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().

doc/api/packages.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Show resolved Hide resolved
Comment on lines 87 to 91
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:

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

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.

[`"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.

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

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

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:

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

@GeoffreyBooth

This comment has been minimized.

doc/api/cli.md Outdated

The program entry point is a specifier-like string. That string is first passed
through `path.resolve()` and the [CommonJS][] modules loader. If no
corresponding module, an error is thrown.
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
corresponding module, an error is thrown.
corresponding module is found, an error is thrown.

A user so new to Node that they’re reading this page is highly unlikely to know what the CommonJS modules loader is, or even path.resolve. Is there a way we can describe what it means for the string to be parsed by those things, that doesn’t assume any prior Node knowledge?

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 all for simplifying what can be simplified, but I also strongly think docs should not omit details on the only basis that new users won't be familiar. I think it's fair to assume no one that has participated in the discussion in this PR is new to Node.js (euphemism), yet folks have expressed surprise at node behavior regarding entry point specifier: to me, that indicates that it should be documented.

Maybe instead we should try to define what is the CommonJS module loader and link to that?

Regarding path.resolve, should we replace that with if it's not an absolute path, it's resolved as a relative path from the current working directory?

Copy link
Member

Choose a reason for hiding this comment

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

Re path.resolve, sure, that sounds good.

Re the loaders, if you want to mention them then I think you need to introduce them. Something like:

Node.js has two module systems: CommonJS and ECMAScript (ES) Modules. A set of rules define whether a particular specifier (the string passed to require or import) should be interpreted as a CommonJS or ES module. Once this determination is made, either the CommonJS or ES module loader Node.js subsystem then resolves the specifier into a file path or URL, and loads the source for that resource.

The last sentence is the key: I’m defining what either of these loaders is by describing what it does. (What it “is” is a Node.js subsystem, but that’s not very informative.)

And within this brief overview, you can link to the detailed sections describing each loader.

Copy link
Contributor Author

@aduh95 aduh95 Jan 7, 2022

Choose a reason for hiding this comment

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

A set of rules define whether a particular specifier (the string passed to require or import) should be interpreted as a CommonJS or ES module. Once this determination is made, either the CommonJS or ES module loader Node.js subsystem then resolves the specifier into a file path or URL, and loads the source for that resource.

A specifier passed to import is always resolved and loaded by the ES module loader (which may in turn pass the resolved URL to the CJS loader), and a specifier passed to require is always resolved and loaded by the CJS loader. I must be missing something, I don't understand what you mean in this last sentence...

Copy link
Member

Choose a reason for hiding this comment

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

From the user's perspective, saying that import is always handled by the ESM loader implies that the reference would always be treated as ESM. What matters is that Node can treat some imports as CommonJS and some as ESM. As in, what Node is doing is more important than what Node subsystem is doing it.

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 will depend if the user is a loader author (or maybe even a loader user) imho. If loaders were not public, I would agree that's just an implementation detail that would not belong in the docs. Unless we want to make separate docs for loaders, I think we should have this information somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

You can say that one loader or the other is responsible, that's fine. My point is to go further and explain what it means that a particular loader handles something: what does it do as a result?

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.

doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
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

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.

Comment on lines 90 to 91
Calling `require()` always use the CommonJS loader, calling `import()` always
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
Calling `require()` always use the CommonJS loader, calling `import()` always
use the [ECMAScript modules][] loader.
Calls to `require()` will always try to load the referenced resource as a
CommonJS module. `import()` expressions will load _either_ an
ES module or a CommonJS module, depending on how
the file being imported should be interpreted.
`import()` can be used to reference ES modules from
CommonJS code.

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 don't think your suggestion is correct, require('./file.json') will first try to load a JSON file before trying to load it as CJS.
I mean, I get that using jargon and expect the reader to know what we mean by "loader" is not a low-bar, but this is a technical documentation, I'd rather use the technical terms than write a potentially mis-leading or incomplete documentation.

Copy link
Member

Choose a reason for hiding this comment

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

If require is loading json, it's loading it as a CommonJS module. Does a CommonJS module need to be JavaScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, that was my understanding but maybe it's wrong and a JSON file can count as a CommonJS module. The lack of official spec makes the definition blurry so it's probably not worth debating it.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Show resolved Hide resolved
doc/api/synopsis.md Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
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.

The grammar is addressed so I'll approve this. I think we can still do a better job making the loaders more explained in introductory terms, if you'd like to keep iterating on the other notes, but this is already good enough to merge I think.

doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
Comment on lines 96 to 97
* When resolving a specifier, if no exact match is found, it will try to add
extensions (`.js`, `.json`, and finally `.node`).
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
* When resolving a specifier, if no exact match is found, it will try to add
extensions (`.js`, `.json`, and finally `.node`).
* When resolving a specifier, if no exact match is found, it will try to add
extensions (`.js`, `.json`, and finally `.node`) and index files
(`/index.js`, `/index.json`, and finally `/index.node`).

Copy link
Contributor Author

@aduh95 aduh95 Jan 13, 2022

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 a package.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)

Copy link
Member

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?

* When resolving a specifier, if no exact match is found, it will try to add
  extensions (`.js`, `.json`, and finally `.node`) and then attempt to resolve
  [folders as modules][].

Copy link
Contributor Author

@aduh95 aduh95 Jan 14, 2022

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

* When resolving a specifier, if no exact match is found, it will try to add
extensions (`.js`, `.json`, and finally `.node`).
* It supports [folders as modules][].

EDIT: no you're right, we should document the order here.

node/doc/api/packages.md

Lines 97 to 100 in fe05b72

* It supports [folders as modules][].
* When resolving a specifier, if no exact match is found, it will try to add
extensions (`.js`, `.json`, and finally `.node`) and then attempt to resolve
[folders as modules][].

doc/api/packages.md Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
Comment on lines 104 to 106
* It cannot be used to load ECMAScript modules. Attempting to do so will result
in a [`ERR_REQUIRE_ESM`][] error.
* It can be accessed using `require` function.
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
* It cannot be used to load ECMAScript modules. Attempting to do so will result
in a [`ERR_REQUIRE_ESM`][] error.
* It can be accessed using `require` function.
* It allows one CommonJS module to load another using the `require` function.
* It allows CommonJS modules to asynchronously import ECMAScript modules,
using `import()` expressions.
* `require` cannot be used to load ECMAScript modules. Attempting to do so
will result in a [`ERR_REQUIRE_ESM`][] error.

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

Copy link
Member

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?

* It allows one CommonJS module to load another using the `require` function.
  `require` cannot be used to load ECMAScript modules. Attempting to do so
  will result in a [`ERR_REQUIRE_ESM`][] error.
* It allows CommonJS modules to asynchronously import ECMAScript modules,
  using `import()` expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows one CommonJS module to load another using the `require` function.

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.

It allows CommonJS modules to asynchronously import ECMAScript modules,
  using `import()` expressions.

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 list import() as a CJS loader feature.

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 went with

node/doc/api/packages.md

Lines 103 to 107 in 21b45bf

* It treats all files that lack `.json` or `.node` extensions as JavaScript
text files.
* It cannot be used to load ECMAScript modules. Attempting to do so will result
in a [`ERR_REQUIRE_ESM`][] error. When used to load a JavaScript text file
that is not an ECMAScript module, it loads it as a CommonJS module.
wdyt?

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 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 in modules.md, I'd prefer if we leave it out to simplify that list item.

what’s the value of stating that the CommonJS loader only loads CommonJS?

ESM loader can load more than just ESM, so it may not be obvious that CJS only loads CJS?

Copy link
Member

Choose a reason for hiding this comment

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

ESM loader can load more than just ESM, so it may not be obvious that CJS only loads CJS?

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 of import() 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 while require 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.

Copy link
Member

Choose a reason for hiding this comment

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

(require only supports CJS)

Copy link
Contributor Author

@aduh95 aduh95 Jan 17, 2022

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; with esmLoader.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 and import are user facing features, and are already documented elsewhere, it's easier if we define require => CJS loader and import => ESM loader (and it's mostly true AFAIK), and I think it would be detrimental to imply that import and require 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.

Copy link
Member

Choose a reason for hiding this comment

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

ESM loader can load more than just ESM, so it may not be obvious that CJS only loads CJS?

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, while import/import() can take either. And that import doesn’t exist in CommonJS, but import() 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 vs import rather than about the loader itself.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
aduh95 and others added 4 commits January 13, 2022 12:39
Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>
doc/api/cli.md Outdated
Comment on lines 37 to 38
* If the program was started with a command-line flag that forces the entry
point to be loaded with ECMAScript module loader.
Copy link
Contributor

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 before node 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).

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)

Copy link
Contributor Author

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.

Copy link
Member

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 .

Copy link
Contributor

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.

@aduh95 aduh95 merged commit 8732591 into nodejs:master Jan 17, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 17, 2022

Landed in 8732591

@aduh95 aduh95 deleted the module-system-selection branch January 17, 2022 10:36
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 <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
Refs: #41345 (comment)

PR-URL: #41383
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
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 <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 26, 2022
Refs: #41345 (comment)

PR-URL: #41383
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

8 participants