Repository navigation
Missed needed breaking change for 12.x #320
Description
Activity
Also worth mentioning that since we have
"type": "module"we have re-introduced the hazard for requiring a module with the.jsextension... should we consider removing this?@MylesBorins are you suggesting removing the "type" field entirely?
@ljharb no, removing the "throw" on require('thing.mjs')
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.mjsextension handler for.js(ie, throws for now, or applies whatever the user-provided.mjshook is) within that scope.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.
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.cjsAssuming
package.jsonhas"type": "module", when we executeindex.jsas esm, when it goes and importsfile.cjsas cjs, whenfile.cjsrunsrequire('./index.js'), it will attempt to execute it as cjs when the package scope clearly indicates it should be esm. Ifindex.jshas no es-module-syntax, this means you'd even up with two cache entries and executing the same code twice - so if the module'sindex.jsis 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.I’m fine with this change. We need it if unflagging
--experimental-modulesmakesrequire('file.mjs')loadfile.mjsas ESM. If this change doesn’t go in, and we do ultimately decide to allowrequireof ESM, it would mean thatrequireof ESM would need to stay flagged until it could debut as a breaking change in 13.If we don’t end up shipping
requireof ESM, then we only need this change so that we can makerequireof ESM throw when we unflag. We could alternatively remove that throw, leavingrequirecompletely unchanged. Then we could at least unflag in 12, and add the throw as a breaking change in 13.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".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.
probably needs to go out of their way
Not really. if I
require('package/file.js')(or even justpackage/file) andpackagehas migrated toesmunder the hood using"type": "module", I've just stumbled headfirst into it. Now, iffilehas 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.landed in d370d126c3
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 conversationnodejs/node#27417