module: unflag detect-module #53619

GeoffreyBooth · 2024-06-28T05:07:10Z

Notable change

Module syntax detection (the --experimental-detect-module flag) is now enabled by default. Use --no-experimental-detect-module to disable it if needed.

Syntax detection attempts to run ambiguous files as CommonJS, and if the module fails to parse as CommonJS due to ES module syntax, Node.js tries again and runs the file as an ES module. Ambiguous files are those with a .js or no extension, where the nearest parent package.json has no "type" field (either "type": "module" or "type": "commonjs"). Syntax detection should have no performance impact on CommonJS modules, but it incurs a slight performance penalty for ES modules; add "type": "module" to the nearest parent package.json file to eliminate the performance cost. A use case unlocked by this feature is the ability to use ES module syntax in extensionless scripts with no nearby package.json.

Description

This is a PR to unflag --experimental-detect-module. It does two things:

It makes the --experimental-detect-module flag enabled by default, letting people disable it via --no-experimental-detect-module.
Per module: resolve format for all situations with auto module detection on #53044 (comment) this PR changes the format hint returned by Node’s internal resolve for ambiguous (no package.json type field, no .mjs or .cjs extension) files from commonjs to null. 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 because of the unflagging:

Not breaking changes

test/es-module/test-esm-cjs-exports.js should error on invalid CJS exports: Previously this was testing for the error strings Warning: To load an ES module and Unexpected token \'export\'; now it tests for the error string SyntaxError: 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.mjs should import when running –check fails: This test checks that an ESM file passed to --import is evaluated with a CommonJS entry passed to --check fails the syntax check. The file passed to --import is run in both cases, though the previous SyntaxError is now a success; but the point of the test is about the --import file being evaluated at all, not whether its evaluation was successful.
test/es-module/test-require-module-implicit.js: Some tests were attempting to require an ambiguous, extensionless ESM file and expecting an exception with Unexpected token 'export'. These tests were removed. This file is run via --experimental-require-module so this change shouldn’t be considered breaking.

Possibly breaking changes

test/es-module/test-esm-detect-ambiguous.mjs should not hint wrong format in resolve hook: Previously this expected a format hint of commonjs from a resolve hook, and now it expects null, an intentional change. The hooks are still experimental and the format hint from resolve is 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.mjs should use ESM loader to respond to require.resolve calls when opting in: The load hook in this test previously assumed that most files being imported got a format of commonjs, whereas now many of them are undefined, an intentional change. Same impact as previous.
test/es-module/test-esm-resolve-type.mjs: Some tests for a format hint of commonjs now check for a format hint of null. 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 via import() 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.

nodejs-github-bot · 2024-06-28T05:07:15Z

Review requested:

@nodejs/loaders

joyeecheung · 2024-06-28T15:01:07Z

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).

GeoffreyBooth · 2024-06-29T03:46:31Z

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 #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.

GeoffreyBooth · 2024-07-04T15:14:29Z

With #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.

nodejs-github-bot · 2024-07-04T15:15:27Z

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

test/es-module/test-esm-extensionless-esm-and-wasm.mjs

nodejs-github-bot · 2024-07-08T06:23:08Z

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

nodejs-github-bot · 2024-07-08T12:36:47Z

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

lib/internal/main/check_syntax.js

nodejs-github-bot · 2024-07-20T18:30:48Z

Landed in 604ce4c

RedYetiDev · 2024-07-20T18:59:49Z

notable-change and/or semver-<minor/major>?

github-actions · 2024-07-20T22:41:41Z

The notable-change PRs with changes that should be highlighted in changelogs. 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.

PR-URL: #53619 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: TODO

Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123

PR-URL: #53619 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: TODO

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452

oliviertassinari · 2024-08-28T17:46:50Z

I love to see this.

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 28, 2024

This comment was marked as resolved.

Sign in to view

GeoffreyBooth mentioned this pull request Jun 28, 2024

Test runner module mocking format #53634

Closed

This comment was marked as resolved.

Sign in to view

GeoffreyBooth force-pushed the unflag-detect-module branch 2 times, most recently from 0fad673 to 28c75eb Compare June 29, 2024 03:44

GeoffreyBooth force-pushed the unflag-detect-module branch from 28c75eb to 6a89aa7 Compare June 29, 2024 04:35

GeoffreyBooth mentioned this pull request Jun 29, 2024

test_runner: support module detection in module mocks #53642

Merged

GeoffreyBooth force-pushed the unflag-detect-module branch from 6a89aa7 to f4893b8 Compare July 4, 2024 15:10

GeoffreyBooth marked this pull request as ready for review July 4, 2024 15:14

benjamingr approved these changes Jul 4, 2024

View reviewed changes

aduh95 reviewed Jul 5, 2024

View reviewed changes

test/es-module/test-esm-extensionless-esm-and-wasm.mjs Outdated Show resolved Hide resolved

GeoffreyBooth force-pushed the unflag-detect-module branch from 6148d9c to 138f787 Compare July 8, 2024 05:36

GeoffreyBooth requested a review from aduh95 July 8, 2024 05:38

GeoffreyBooth force-pushed the unflag-detect-module branch from 138f787 to f9e3340 Compare July 8, 2024 05:49

joyeecheung reviewed Jul 8, 2024

View reviewed changes

lib/internal/main/check_syntax.js Outdated Show resolved Hide resolved

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 20, 2024

GeoffreyBooth added notable-change PRs with changes that should be highlighted in changelogs. and removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Jul 20, 2024

GeoffreyBooth deleted the unflag-detect-module branch July 20, 2024 22:41

RafaelGSS mentioned this pull request Jul 30, 2024

v22.6.0 proposal #54123

Merged

dygabo mentioned this pull request Aug 1, 2024

module: resolve format for all situations with auto module detection on #53044

Closed

RafaelGSS added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Aug 5, 2024

RafaelGSS mentioned this pull request Aug 5, 2024

module: do not warn for typeless package.json when there isn't one #54045

Merged

RafaelGSS mentioned this pull request Aug 12, 2024

Node.js Technical Steering Committee (TSC) Meeting 2024-08-14 nodejs/TSC#1606

Open

mcollina added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Aug 14, 2024

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

joyeecheung mentioned this pull request Sep 16, 2024

module: implement the "module" exports condition #54648

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: unflag detect-module #53619

module: unflag detect-module #53619

GeoffreyBooth commented Jun 28, 2024 •

edited

Loading

nodejs-github-bot commented Jun 28, 2024

This comment was marked as resolved.

joyeecheung commented Jun 28, 2024 •

edited

Loading

This comment was marked as resolved.

GeoffreyBooth commented Jun 29, 2024 •

edited

Loading

GeoffreyBooth commented Jul 4, 2024

nodejs-github-bot commented Jul 4, 2024

nodejs-github-bot commented Jul 8, 2024

nodejs-github-bot commented Jul 8, 2024

nodejs-github-bot commented Jul 20, 2024

RedYetiDev commented Jul 20, 2024 •

edited

Loading

github-actions bot commented Jul 20, 2024

oliviertassinari commented Aug 28, 2024

module: unflag detect-module #53619

module: unflag detect-module #53619

Conversation

GeoffreyBooth commented Jun 28, 2024 • edited Loading

Notable change

Description

Not breaking changes

Possibly breaking changes

nodejs-github-bot commented Jun 28, 2024

This comment was marked as resolved.

joyeecheung commented Jun 28, 2024 • edited Loading

This comment was marked as resolved.

GeoffreyBooth commented Jun 29, 2024 • edited Loading

GeoffreyBooth commented Jul 4, 2024

nodejs-github-bot commented Jul 4, 2024

nodejs-github-bot commented Jul 8, 2024

nodejs-github-bot commented Jul 8, 2024

nodejs-github-bot commented Jul 20, 2024

RedYetiDev commented Jul 20, 2024 • edited Loading

github-actions bot commented Jul 20, 2024

oliviertassinari commented Aug 28, 2024

GeoffreyBooth commented Jun 28, 2024 •

edited

Loading

joyeecheung commented Jun 28, 2024 •

edited

Loading

GeoffreyBooth commented Jun 29, 2024 •

edited

Loading

RedYetiDev commented Jul 20, 2024 •

edited

Loading