module: allow unrestricted cjs requires

[guybedford]

This implements a follow-up to the issue we had in nodejs/modules#389 with the merge of #29492 and later correction in #29732.

The approach taken is to allow CommonJS modules to require any other files, even .js files in a "type": "module" boundary to ensure maximum backwards compatibility. require of .mjs files is still not permitted though without manually overridding require._extensions['.mjs']. To retain the module format invariants we specifically then do not inject CJS loads into the ESM loader when they break the expected module format, which is the check implemented here.

Technically this allows the module registries to diverge in that we can have the same module path in both the CJS and ESM registries, but meaning different things. Note that only a module not using module syntax or require would cause this though as it would be a failure in either registry otherwise. Because of these syntax restrictions and the top-level main restrictions remaining the same (this only applies from require()), the ways this "technical incompleteness" causes real issues is impractically small such that the usability and compatibility benefits seem worth it to me.

I expect this will be somewhat controversial and we will need to add this to the modules agenda though.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[guybedford]

//cc @nodejs/modules-active-members

[jkrems]

Assuming we get consensus in the modules group, this LGTM

[weswigham]

Even beyond the safety guarantees, if .js can't be relied upon to imply "module"-ness in a type: module scope, tooling cannot reliably determine the intended format of a file, which was the reason for the flag in the first place. IMO, this change makes type: module nearly unshippable, by removing half the meaning of the flag. I won't argue for that, but I won't be able to disagree with anyone who does.

I firmly believe that there are no compat concerns for require in packages which have type: module set - that flag very explicitly opts one in to new, different resolver behavior.

[guybedford]

@weswigham the assumption for tooling should be the rules of the ESM resolver. This is the well-defined semantics we are placing down. That CommonJS can load a .exe file as JS if it wants is the way CommonJS works. For backwards compatibility I don't think we should be placing restrictions on backwards-compatibility space. Tooling will always need customization options, but the default as we've defined for the ESM resolver will become the stable default over time. In the interim though I'd prefer to not break edge cases for users.

[weswigham]

Like, without this, static analysis tools like TS are forced to use heuristic syntax based format detection, which was one of the things the field was supposed to avoid - except this is even worse, since it admits the middle ground where modules are valid under both execution environments, rather than formalizing what syntax indicates what format. If a tool is asked to transpile such a module, what is one to do? Assume it's strict mode? Assume it is not? You can choose to dismiss such cases, but our original discussions appeared to weigh those edge cases quite highly, and, if they are being dismissed, more complete alternatives like automatic format detection based on syntax baked into the engine (so there was an authorative specification from which to derive an answer) would both align with existing tooling better and make for a better user experience.

[weswigham]

This isn't an interim solution - this is a walk back of half of type: module. I would not have in good conscience come to consensus on it being designed like this, given the impact it would have on tooling. Yes, there is a point in time issue that naive tools currently unaware of type: module do not load config files (that are incorrectly assumed to be cjs) correctly, but that's a much easier hurdle to clear than the problem of forever being unsure of what format a file is!

[guybedford]

@weswigham thanks for explaining your case in more detail, I see it presents the ambiguity we were trying to avoid for you.

@GeoffreyBooth has suggested we make this an explicit deprecation warning. I could get behind that as an approach that helps guide users to the right approaches. Would that work for you here?

[weswigham]

has suggested we make this an explicit deprecation warning. I could get behind that as an approach that helps guide users to the right approaches. Would that work for you here?

Make what a deprecation warning? requireing a JS file in a type: module scope? That's a good middleground between what we have in the head tree now (where the error's only issued when the experimental flag is set) and what was in the tree last month (which always issued an error). However it raises an import timing question of when that deprecation warning becomes reality - clearly deprecations can't be acted upon in a minor release, which means this wouldn't be acted upon until node 13, which means the modules implementation would unflag with unsafe behavior in place, for the sake of expediency. The point of getting the error in was so we could unflag with it, to avoid this situation, and brings into question if we can ship type: module in node 12 at all.

[GeoffreyBooth]

My thinking would be that the deprecation warning becomes an error in Node 13/14. The intent of the warning was more to preserve the possibility of require of ESM in the future.

I don’t think that require of CommonJS .js within type: module is “unsafe behavior” such that we can’t ship type: module. A tool shouldn’t be doing it, and a warning will prompt people to update the tool (or watch it start failing in Node 13). But that’s all, and I think that’s sufficient for the minor danger presented here.

[weswigham]

I don’t think that require of CommonJS .js within type: module is “unsafe behavior” such that we can’t ship type: module.

Except by the rules we've established, .js within type: module is never meant to be commonjs - this just muddies the waters for the duration of an entire LTS release (which is a long time, which is why we wanted to have it in place for it!).

And it's not like there aren't other options for people waiting on tools to update - simply not using type: module (ie, using .mjs) until your toolchain supports it properly is sufficient - this half-measure from node itself hurts the ecosystem, by introducing a transitional period with a 3rd (admittedly deprecated) set of behaviors. I, personally, would never want to ship a new feature with a deprecation already in place - it indicates that the feature is rushed, IMO.

[GeoffreyBooth]

For reference, all that tools need to do to bypass the error (as in, do the “bad” behavior, of loading .js within type: module as CommonJS) is this: https://github.com/eslint/eslint/pull/12333/files. Regardless of how strong we intend type: module (or .mjs) to be, they’re very easily overridden.

The issue is really that users add type: module without understanding the consequences, and they don’t connect that this change breaks a tool that they’re using. A better error or warning message could explain this, and guide users on how to resolve their problem (e.g., remove type: module or stop using <name of package that error was thrown inside>).

(fixed, mostly)

[guybedford]

Per the last meeting, we are going with a warning as in #29909.