Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: unflag detect-module #53619

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented Jun 28, 2024

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.

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. 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 Jun 28, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot 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
@cjihrig

This comment was marked as resolved.

@joyeecheung
Copy link
Member

joyeecheung commented Jun 28, 2024

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

This comment was marked as resolved.

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

GeoffreyBooth commented Jun 29, 2024

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
Copy link
Member Author

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.

@GeoffreyBooth GeoffreyBooth marked this pull request as ready for review July 4, 2024 15:14
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 20, 2024
@GeoffreyBooth 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 GeoffreyBooth deleted the unflag-detect-module branch July 20, 2024 22:41
Copy link
Contributor

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.

targos pushed a commit that referenced this pull request Jul 28, 2024
PR-URL: #53619
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS added a commit that referenced this pull request Jul 30, 2024
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
@RafaelGSS RafaelGSS mentioned this pull request Jul 30, 2024
RafaelGSS added a commit that referenced this pull request Jul 30, 2024
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
@RafaelGSS 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
@mcollina 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 RafaelGSS added lib / src Issues and PRs related to general changes in the lib or src directory. and removed lib / src Issues and PRs related to general changes in the lib or src directory. 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 pushed a commit that referenced this pull request Aug 19, 2024
PR-URL: #53619
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 19, 2024
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
RafaelGSS added a commit that referenced this pull request Aug 19, 2024
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
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
RafaelGSS added a commit that referenced this pull request Aug 20, 2024
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
RafaelGSS added a commit that referenced this pull request Aug 21, 2024
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
RafaelGSS added a commit that referenced this pull request Aug 22, 2024
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
Copy link

I love to see this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.