module: error for CJS .js load within type: module

[guybedford]

This adds the CJS error that a .js file within type: module package scopes cannot be loaded from CJS.

This was a resolution made at the last Node.js modules meeting (see notes).

The error thrown is the same one thrown for loading .mjs files from CJS.

//cc @nodejs/modules-active-members

[jkrems]

LGTM. Note: This does technically introduce a subtle change in the main resolution: If the package.json gets created after the initial attempt, this will now be hidden by the cache. Previously it wasn't caching the "no package.json" case for main.

Example:

1. Try to load foo which doesn't exist.
2. Create node_modules/foo.
3. Retry loading foo.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25426/

[weswigham]

Also: Does the esm loader currently read a index.js before a index.mjs (if both are present) when type: module is unset and import the index.js as cjs? That's also needed.

[weswigham]

Should this error be behind the flag for now, or are we OK with throwing here without the --experimental-modules flag set?

[guybedford]

Just like we don't check the flag for loading from .mjs, it seems important to include unflagged as soon as possible to ensure we are reserving the compatibility space, unless you feel strongly about this?

[jkrems]

I think we want this unflagged or at the very least using a different flag. This is cleaning up an inconsistency with how .mjs and .js are handled so if throwing on .mjs isn't behind a flag, neither should this imo (and vice versa if I misremembered the current state :)).

[weswigham]

it seems important to include unflagged as soon as possible to ensure we are reserving the compatibility space, unless you feel strongly about this?

Nah, I'm fine with it (hence the approve), just wanted to ask if it had been considered.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25496/

[Trott]

Multiple failures in CI for test/parallel/test-policy-parse-integrity.js. Are they likely related?

Example:

08:32:30 not ok 1491 parallel/test-policy-parse-integrity
08:32:30   ---
08:32:30   duration_ms: 0.173
08:32:30   severity: fail
08:32:30   exitcode: 1
08:32:30   stack: |-
08:32:30     assert.js:89
08:32:30       throw new AssertionError(obj);
08:32:30       ^
08:32:30     
08:32:30     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
08:32:30     
08:32:30     1 !== 0
08:32:30     
08:32:30         at test (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/parallel/test-policy-parse-integrity.js:64:12)
08:32:30         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/parallel/test-policy-parse-integrity.js:68:1)
08:32:30         at Module._compile (internal/modules/cjs/loader.js:939:30)
08:32:30         at Object.Module._extensions..js (internal/modules/cjs/loader.js:954:10)
08:32:30         at Module.load (internal/modules/cjs/loader.js:792:32)
08:32:30         at Function.Module._load (internal/modules/cjs/loader.js:705:12)
08:32:30         at Function.Module.runMain (internal/modules/cjs/loader.js:1006:10)
08:32:30         at internal/main/run_main_module.js:17:11 {
08:32:30       generatedMessage: true,
08:32:30       code: 'ERR_ASSERTION',
08:32:30       actual: 1,
08:32:30       expected: 0,
08:32:30       operator: 'strictEqual'
08:32:30     }
08:32:30   ...

[Trott]

@bmeck ^^^^^^

[bmeck]

package.json files got added to the test, thats probably making it angry, i'm not entirely sure why they were added but on a glance it doesn't look obvious whats wrong.

[bmeck]

this seems to not handle certain types of package.json

{"type": "module"}

won't fire this change. I assume this package.json above should still fire this error.

nit: additionally should this be fired for extension-less files and/or any files not picked up by ESM mappings to types?

[guybedford]

@bmeck the test here is exactly for a { "type": "module" } package JSON giving this error. Can you clarify further what you mean by not firing?

Good questions re other extensions. ESM doesn't permit unknown extensions (as in it will throw), so there is no longer an ambiguity for those cases as it is clear they can only be loaded as CommonJS. This PR was specifically about handling the .js dual mode ambiguity between CJS and ESM.

If you feel strongly that they should throw for unknown extensions in "type": "module" for CJS we can consider that further certainly.

[bmeck]

@guybedford I did: echo '{"type":"module"}' > package.json && echo '' > a.js then opened the node REPL and did require('./a.js'), it didn't seem to error at all.

[guybedford]

Ah, the JSON property filter needed an entry to handle a package.json without "main" or "exports", I've added a commit with that fix.

[guybedford]

(This is still likely unrelated to CI failure though unfortunately)

fixed issue, CI still unclear

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25552/

[guybedford]

Finally tracked down the issue here, and it was an obvious problem with the policy test after all. Still not sure why it was flaky though as it was a package.json integrity check availability issue.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25561/

[NemoStein]

As noted in multiple issues, this change creates a lot of incompatibilities.
One issue that I didn't found reported anywhere is related to bin scripts.

In a {"type": "module"} package, in node 12.10.0, a bin script couldn't use import without the experimental-modules flag, forcing the developer to use require.
In node 12.11.0 this behavior is inverted. A bin script must load dependencies with import and throws if require is used.

The only workaround so far is renaming the files to *.cjs, but TypeScript gets mad with this change (e.g.: IntelliSense doesn't work).

Please, move cjs type check behind experimental-modules flag or lift this check for bin scripts.

[jkrems]

It also sounds like other tools may have introduced incompatible assumptions about the type field. So maybe node needs to "just" pick another property name if we want to preserve the meaning of "changes the parse goal of all .js files in the scope".

[weswigham]

So maybe node needs to "just" pick another property name if we want to preserve the meaning of ...

I think it's less about that, and moreso that @NemoStein already adopted {"type": "module"} for --experimental-module support, but in doing so this change changed his code behavior when --experimental-module is not set. You could call it a flaw because we didn't ship with this behavior in, but IMO, it was expected, {"type": "module"} package.json field support (even w/o a flag) is still experimental.

The name of the field is irrelevant - given the same order of events, the same thing could have happened with any name - all that could have avoided his breakage would have been only revealing the field name as the errors related to its' use were added. In a way, this is similar to the concerns tc39 had with globalThis - that once they revealed the name, people would start writing polyfills that were incompatible...

[jkrems]

I wasn't necessarily talk about the one comment, more about the overall situation. One of them is:

1. Use type: module in package.json.
2. Use @babel/register to run and/or test the code.
3. The transpiled code for the .js files ends up in require.extensions[.js] which rightfully thinks the input is ESM (given the package.json flag).

This project will not work after unflagging. It will still register babel, it will still "fail" to meet node's current assumption about type: module by providing CJS to node in a context where node expects ESM.

[GeoffreyBooth]

Any tool that hooks into CommonJS internals, especially something using require.extensions either directly or through something like a register hook, is going to have issues with type: module or ESM generally. That’s just unavoidable. Like I wrote elsewhere, users will just have to choose between type: module (or even Node using ESM syntax for their project) and whatever tools have incompatibilities, until those tools are updated.

Since type: module and more generally, Node evaluating ESM syntax, are both opt in, we don’t need to solve every use case for every incompatible tool before we unflag. Users can just keep doing what they’re doing, and not add type: module or start using .mjs or whatever, until all the tools they want to use are updated to be fully compatible.

[GeoffreyBooth]

@weswigham One downside of removing the error on CommonJS require of .js inside a type: module scope is that then require of ESM becomes a breaking change. It doesn’t seem likely to me that we’ll ship require of ESM as part of Node 12 anyway, so what if we change this error to a deprecation warning? And then in Node 14, CommonJS require of .js inside a type: module scope would throw an error, whether or not we have require of ESM implemented by then.

[weswigham]

One downside of removing the error on CommonJS require of .js inside a type: module scope is that then require of ESM becomes a breaking change.

The error is still here, it's just being moved to behind the experimental-modules flag. That implies, if you share the sentiment that this error is a "breaking change", that unflagging esm is thus a breaking change.

[weswigham]

On the other hand, since type: module is opt-in, I really don't think anything related to it counts as a breaking change, really.

[weswigham]

It doesn’t seem likely to me that we’ll ship require of ESM as part of Node 12 anyway, so what if we change this error to a deprecation warning?

Also, require of esm isn't a breaking change with this, it's type: module that is. require("whatever/f.mjs") (thereby not relying on type: module) would still theoretically work fine.

If this change concerns you with it's breakiness, then you're concerned with not shipping .js for esm, since that's what being unable to unflag with type: module implies.

[GeoffreyBooth]

@weswigham Sorry I think I was unclear. I meant if when we unflag, if this error isn’t present, then later adding require of ESM would be a breaking change. As in, require of .js under type: module would suddenly go from treating the .js as CommonJS to treating it as ESM. That’s the breaking change I was referring to, the potential breaking change if we don’t ship with this error.

I think we should either ship with this error when we unflag, and it’ll be painful for tools for awhile; or we at least ship with a deprecation warning so that we can ship require of ESM in Node 14.

[weswigham]

That’s the breaking change I was referring to, the potential breaking change if we don’t ship with this error.

I don't think the behavior of require under a type: module scope is anything other than experimental right now, and as such cannot be viewed as a breaking change, tbh. The reason the package.json field was added was to allow people to opt-in to the new behavior (which is still being worked on), so if the behavior changes between patches, caveat empor - you're using an unstable feature.

[GeoffreyBooth]

I'm not referring to current opt-in behavior. I'm referring to someday adding require of ESM and having that not be a breaking change. Once type module is unflagged, I want us to be able to still add require of ESM after that point.

[weswigham]

Even without require of esm it'd be a breaking change, if that's your perspective, as it'd go from being interpreted as cjs to being an error.

[GeoffreyBooth]

Even without require of esm it'd be a breaking change, if that's your perspective, as it'd go from being interpreted as cjs to being an error.

That's why when we unflag I want to either keep the error or make it a deprecation warning. Then we can add require of ESM in Node 14 and it either wouldn't be considered a breaking change (as something going from an error to working is generally allowed as non-breaking) or it would be an allowed breaking change if it went from a deprecation warning to working differently.

[weswigham]

Node 14

Literally does not matter then (you can do anything with a major version bump) does it? What matters is what we can unflag with.

[GeoffreyBooth]

Literally does not matter then (you can do anything with a major version bump) does it?

I suppose; but if we don't warn the community to update their code, we'll have this same issue in Node 14 where tools are "broken" by the change. If they get months or years of warnings first, they might be updated before 14 is released.