GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: unflag detect-module by GeoffreyBooth · Pull Request #53619 · nodejs/node

Conversation

@GeoffreyBooth

@GeoffreyBooth GeoffreyBooth commented Jun 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danielbayley reacted with thumbs up emoji avivkeller, MomenNano, bricss, xav-ie, kirillgroshkov, and oliviertassinari reacted with heart emoji marco-ippolito, avivkeller, MomenNano, mcollina, bricss, xav-ie, and kirillgroshkov reacted with rocket emoji ganeshkbhat reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • ❤️ 6 reactions
  • 🚀 7 reactions
  • 👀 1 reaction
@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 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

nodejs-github-bot commented Jun 28, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
ganeshkbhat reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

This comment was marked as resolved.

Sign in to view
@joyeecheung

joyeecheung commented Jun 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth GeoffreyBooth mentioned this pull request Jun 28, 2024
Test runner module mocking format #53634
Closed
@GeoffreyBooth

This comment was marked as resolved.

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

GeoffreyBooth commented Jun 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

dygabo reacted with thumbs up emoji ganeshkbhat reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth
GeoffreyBooth force-pushed the unflag-detect-module branch from 28c75eb to 6a89aa7 Compare June 29, 2024 04:35
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jun 29, 2024
test_runner: support module detection in module mocks #53642
Merged
@GeoffreyBooth
GeoffreyBooth force-pushed the unflag-detect-module branch from 6a89aa7 to f4893b8 Compare July 4, 2024 15:10
@GeoffreyBooth

GeoffreyBooth commented Jul 4, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

nodejs-github-bot commented Jul 4, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr approved these changes Jul 4, 2024
View reviewed changes
aduh95
aduh95 reviewed Jul 5, 2024
View reviewed changes
Comment thread test/es-module/test-esm-extensionless-esm-and-wasm.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth
GeoffreyBooth force-pushed the unflag-detect-module branch from 6148d9c to 138f787 Compare July 8, 2024 05:36
@GeoffreyBooth
GeoffreyBooth requested a review from aduh95 July 8, 2024 05:38
@GeoffreyBooth
GeoffreyBooth force-pushed the unflag-detect-module branch from 138f787 to f9e3340 Compare July 8, 2024 05:49
@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung
joyeecheung reviewed Jul 8, 2024
View reviewed changes
Comment thread lib/internal/main/check_syntax.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 removed the baking-for-lts PRs whose changes need more time before an LTS backport. label Feb 3, 2025
@joyeecheung joyeecheung added backport-open-v20.x and removed dont-land-on-v20.x labels Feb 6, 2025
@joyeecheung

joyeecheung commented Feb 6, 2025

Copy link
Copy Markdown
Member

Backport in #56927

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 6, 2025
@GeoffreyBooth @joyeecheung
module: unflag detect-module
e449780
PR-URL: nodejs#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]>
@joyeecheung joyeecheung mentioned this pull request Feb 7, 2025
[v20.x] backport unflagging of require(esm) to v20 #56927
Closed
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 10, 2025
@GeoffreyBooth @joyeecheung
module: unflag detect-module
bea73c8
PR-URL: nodejs#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]>
marco-ippolito pushed a commit that referenced this pull request Feb 11, 2025
@GeoffreyBooth @marco-ippolito
module: unflag detect-module
9916458
PR-URL: #53619
Backport-PR-URL: #56927
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]>
Refs: #52697
@marco-ippolito marco-ippolito mentioned this pull request Mar 6, 2025
v20.19.0 proposal #57349
Merged
This was referenced Mar 17, 2025
Update product-os/flowzone action to v19.11.13 klutchell/balena-mediaserver#1061
Merged
Update product-os/flowzone action to v19.11.12 klutchell/dnscrypt-proxy-docker#537
Merged
Update product-os/flowzone action to v19.11.12 klutchell/unbound-docker#568
Merged
Update product-os/flowzone action to v19.11.12 klutchell/balena-exit-node#270
Merged
This was referenced Apr 2, 2025
Update product-os/flowzone action to v19.13.15 klutchell/dnscrypt-proxy-docker#544
Merged
Update product-os/flowzone action to v19.13.20 klutchell/balena-mediaserver#1082
Merged
Update product-os/flowzone action to v19.13.20 klutchell/unbound-docker#574
Merged
Update product-os/flowzone action to v19.13.20 klutchell/balena-exit-node#277
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jun 25, 2026
Initialize CodeM8 project b4prog/CodeM8#1
Merged
@colinhacks colinhacks mentioned this pull request Jul 10, 2026
feat(runtime): backport --experimental-detect-module for ambiguous ESM .js nubjs/nub#400
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Jul 11, 2026
Fix Codex Desktop CLI discovery fhajjej-ship-it/loop-it#24
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Aug 6, 2026
feat(#115): cold-installed public role matrix (owner README gate remaining) Akagilnc/ak-pi-workflow-roles#175
Merged
This was referenced Aug 21, 2026
Certify complete tracker reads for TODO import and export unbraind/pm-todos#55
Merged
ONM-20: Enforce fixer prompt guardrails against test assertion mutation escidmost/orca-no-mistakes#18
Merged
@Arylmera Arylmera mentioned this pull request Oct 17, 2026
fix(node): raise the Node floor to 22.7.0 for ESM syntax detection Arylmera/Geneseed#303
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChALkeR ChALkeR ChALkeR left review comments
@joyeecheung joyeecheung joyeecheung left review comments
@avivkeller avivkeller avivkeller left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@benjamingr benjamingr benjamingr approved these changes
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@dygabo dygabo dygabo left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

14 participants

@GeoffreyBooth @nodejs-github-bot @cjihrig @joyeecheung @richardlau @avivkeller @oliviertassinari @mcollina @ChALkeR @jasnell @benjamingr @aduh95 @dygabo @RafaelGSS
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.