nodejs
module: unflag detect-module
This is a draft PR to unflag --experimental-detect-module. I’m labeling it as don’t land for all current lines unless and until we determine that this isn’t a semver-major change; it should be okay to release as part of 23.0.0, and after that we can decide whether or not to backport.
This PR specifically does two things:
- It turns the
--experimental-detect-moduleflag into a noop similar to--experimental-modules. - Per https://github.com/nodejs/node/pull/53044#issuecomment-2195881734 this PR changes the
formathint returned by Node’s internalresolvefor ambiguous (nopackage.jsontypefield, no.mjsor.cjsextension) files fromcommonjstoundefined. This is more correct, as the format hasn’t yet been determined at the time of resolution, because with detection enabled we need to load the source and parse it to detect the format.
I also updated the tests (with one exception: https://github.com/nodejs/node/issues/53634) because of the unflagging:
Not breaking changes
-
test/es-module/test-esm-cjs-exports.jsshould error on invalid CJS exports: Previously this was testing for the error stringsWarning: To load an ES moduleandUnexpected token \'export\'; now it tests for the error stringSyntaxError: The requested module './invalid-cjs.js' does not provide an export named 'default'. Arguably the new version is more appropriate as it’s testing the actual thing the test describes rather than testing an error that was thrown before getting to the invalid CommonJS exports. -
test/es-module/test-esm-import-flag.mjsshould import when running --check fails: This test checks that an ESM file passed to--importis evaluated with a CommonJS entry passed to--checkfails the syntax check. The file passed to--importis run in both cases, though the previousSyntaxErroris now a success; but the point of the test is about the--importfile being evaluated at all, not whether its evaluation was successful. -
test/es-module/test-require-module-implicit.js: Some tests were attempting torequirean ambiguous, extensionless ESM file and expecting an exception withUnexpected token 'export'. These tests were removed. This file is run via--experimental-require-moduleso this change shouldn’t be considered breaking.
Possibly breaking changes
-
test/es-module/test-esm-detect-ambiguous.mjsshould not hint wrong format in resolve hook: Previously this expected aformathint ofcommonjsfrom aresolvehook, and now it expectsundefined, an intentional change. The hooks are still experimental and theformathint fromresolveis documented as optional, so hooks should be written to expect it not to be present; but the unflagging would cause some code that currently returns a hint to no longer do so. -
test/es-module/test-esm-loader-hooks.mjsshould use ESM loader to respond to require.resolve calls when opting in: Theloadhook in this test previously assumed that most files being imported got aformatofcommonjs, whereas now many of them areundefined, an intentional change. Same impact as previous. -
test/es-module/test-esm-resolve-type.mjs: Some tests for aformathint ofcommonjsnow check for aformathint ofundefined. Same impact as previous. -
test/es-module/test-esm-extensionless-esm-and-wasm.mjs:extensionless ES modules within no package scope: tests for ambiguous extensionless ESM running as the entry point and ambugious extensionless ESM running viaimport()used to test for exceptions being thrown; now they test for success. Anyone expecting exceptions on running or importing ambiguous or extensionless ESM (for example, to do detection themselves) will be broken by this change.
Regarding the module mocking - the mock loader (lib/test/mock_loader.js) uses that information to determine whether to generate source code in CJS or ESM. I think there are a few options to fix it:
- Maybe it doesn't really matter and it can be removed?
- Pick a default when the hint is undefined.
- Make it an option that the user can provide as input.
I wonder for hook users relying on the inaccurate context.format whether they should just do const format = context.format || 'commonjs' because that's normally how it goes. Or we could just maintain an "tentative format" internally in addition to the existing format which is more like "confirmed format" and maintain context.format as the "tentative format", while adding context.confirmedFormat or something that carries around format information that are confirmed by the previous resolution & won't incur any further detection in the default load step (and it would be undefined for format that are pending detection and can only be confirmed after the source code is final).
Regarding the module mocking . . . I think there are a few options to fix it:
I think the mocking thing might need a change to the mocking feature itself, more than just a test update, so I created https://github.com/nodejs/node/issues/53634 to discuss the various approaches.
Update: I figured out the require.resolve test. The load hook had an if (result.format === 'commonjs') condition that needed updating to allow for an undefined format. Now all that remains is https://github.com/nodejs/node/issues/53634.
I’ve updated the top post with a list of tests that were updated, grouped by whether I think they should be considered nonbreaking or might be considered breaking.
With https://github.com/nodejs/node/pull/53642 landed, now all the tests pass. This is ready for review.
Assuming we think this is ready to land, the other question we need to answer is how to release it. I updated the top post with my analysis of each changed test and whether the change might be considered breaking or not; basically I think three are definitely not breaking and three are arguably breaking but possibly not. I’m curious to hear what others think. If we think that some of these arguably breaking ones are breaking, then detection can only be unflagged in >23; otherwise I think this should be acceptable to backport.
We can always land it in 23.0.0 first to see the reaction and only backport it afterward, at the cost of delaying the rollout to the <23 lines until November at the earliest.
- It turns the
--experimental-detect-moduleflag into a noop similar to--experimental-modules.
If we think that some of these arguably breaking ones are breaking, then detection can only be unflagged in >23; otherwise I think this should be acceptable to backport.
If we do decide this can be backported, instead of making the flag a noop, maybe swap the default so it defaults to being enabled but can still be disabled via --no-experimental-detect-module?
If we do decide this can be backported, instead of making the flag a noop, maybe swap the default so it defaults to being enabled but can still be disabled via
--no-experimental-detect-module?
It wouldn’t be experimental anymore, so I guess the flags would be --detect-module / --no-detect-module, but yes it’s an option. I’d need to redo this PR to preserve the references to the flag and just change the default, but I could do that.
The first three tests under “Possibly breaking changes” all concern the module customization hooks, which are themselves experimental, so I think that those changes are fair game. It’s the last one, where import(fixtures.fileURL('es-modules/noext-esm')) and node ./test/fixtures/es-modules/noext-esm would previously error and now they work, that is the one that I’m unsure about. Reading https://github.com/nodejs/node/blob/main/doc/contributing/collaborator-guide.md#breaking-changes, is something that used to error changing to no longer error considered a breaking change?
It wouldn’t be experimental anymore, so I guess the flags would be --detect-module / --no-detect-module, but yes it’s an option.
You can keep the experimental alias and add a non-prefixed one, I think we did that for other experimental flags too.
I’ve updated the docs. What are we thinking re backporting and flags?
- Unflag it in 23.0.0, wait a bit to see if anyone complains, then backport the unflagging.
- Unflag it in 22.x and hope for the best, wait for reaction and then continue backporting the unflagging.
- Unflag it in 23.0.0 and also create a new flag
--no-detect-moduleto disable it; wait for reaction and then backport. - Unflag it in 22.x and also create a new flag
--no-detect-moduleto disable it; wait for reaction and then continue backporting. - Rename the flag to
--detect-module, land that in 22.x and below, flip the default in 23.0.0 (so >=23 would have--no-detect-moduleto disable). - Other options?
Per discussion in https://github.com/nodejs/TSC/issues/1592, adding this to the agenda to be discussed more in the next TSC meeting.
I think we have a tentative consensus to:
- Keep the
--experimental-detect-module/--no-experimental-detect-moduleflags, but flip the default from the current off by default to on by default. (So I’ll change this PR, which turns the flag into a noop, into just a change of default.) - Land on 22.
I’ll leave this open until next week’s TSC meeting.
I’ll change this PR, which turns the flag into a noop, into just a change of default.
I’ve implemented this change, if folks would be so kind to review. I’ll leave this open until Wednesday regardless.
@aduh95 I moved the format refactoring into a separate commit on this PR’s branch. How about we land via commit-queue-rebase to put both commits on main, rather than splitting into two PRs?
If that’s okay with you, I think I’ve resolved all your notes.
The https://github.com/nodejs/node/labels/notable-change label has been added by @GeoffreyBooth.
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
