Missed needed breaking change for 12.x

[MylesBorins]

In a call with @weswigham today it became apparent that we likely should have unflagged throwing on require('./path.mjs'), as it is arguably a breaking change to require. I've opened a PR to get the behavior change landed ASAP as Semver-Patch... under the guise that this should have landed in 12.x... it is a stretch, but we should have this conversation

nodejs/node#27417

[MylesBorins]

Also worth mentioning that since we have "type": "module" we have re-introduced the hazard for requiring a module with the .js extension... should we consider removing this?

[ljharb]

@MylesBorins are you suggesting removing the "type" field entirely?

[MylesBorins]

@ljharb no, removing the "throw" on require('thing.mjs')

[weswigham]

FWIW, it should also be non-breaking, IMO, to simply respect the "type": "module" field in the CJS loader (since the field is a new addition). This would mean that if the CJS loader finds a package-scoped "type": "module" setting, it uses the .mjs extension handler for .js (ie, throws for now, or applies whatever the user-provided .mjs hook is) within that scope.

[ljharb]

The semantics and naming of the field aren't set in stone; i don't think it'd be a good idea for it to affect anything without the flag.

[weswigham]

It's worth noting the hazard also only exists within a package with "type": "module" set, which we've only got support for behind a flag right now.

For anyone unsure, the hazard is this:

- package.json
- index.js
- file.cjs

Assuming package.json has "type": "module", when we execute index.js as esm, when it goes and imports file.cjs as cjs, when file.cjs runs require('./index.js'), it will attempt to execute it as cjs when the package scope clearly indicates it should be esm. If index.js has no es-module-syntax, this means you'd even up with two cache entries and executing the same code twice - so if the module's index.js is a side-effecting module (eg, patches the global, pings the net, whatever) it will, surprisingly, be done twice.

Again, since "type": "module" is still flagged, you can't actually encounter this issue unflagged today.

[weswigham]

@ljharb I think the cjs resolver behavior for "type": "module" can be behind the flag until it's done, tbh - I just think it does need it.

[GeoffreyBooth]

I’m fine with this change. We need it if unflagging --experimental-modules makes require('file.mjs') load file.mjs as ESM. If this change doesn’t go in, and we do ultimately decide to allow require of ESM, it would mean that require of ESM would need to stay flagged until it could debut as a breaking change in 13.

If we don’t end up shipping require of ESM, then we only need this change so that we can make require of ESM throw when we unflag. We could alternatively remove that throw, leaving require completely unchanged. Then we could at least unflag in 12, and add the throw as a breaking change in 13.

[weswigham]

We could alternatively remove that throw, leaving require completely unchanged. Then we could at least unflag in 12, and add the throw as a breaking change in 13.

That wouldn't be great, since it introduces the same execution hazard I described above, but without ever opting into "type": "module".

[GeoffreyBooth]

That hazard is the same one I was writing about in #317 (comment), about double instantiation. The tldr is basically that the user probably needs to go out of their way to cause the hazard, so it’s not worth trying to protect them against it.

[weswigham]

probably needs to go out of their way

Not really. if I require('package/file.js') (or even just package/file) and package has migrated to esm under the hood using "type": "module", I've just stumbled headfirst into it. Now, if file has esm syntax it'll at least throw, but it might not, and in those cases, particularly insidious hard to debug things can happen involving duplicated side-effects.

[MylesBorins]

landed in d370d126c3