src: allow disabling JS source phase imports

[codebytere]

Refs:

• https://chromium-review.googlesource.com/c/v8/v8/+/7003082
• https://chromium-review.googlesource.com/c/chromium/src/+/6336964
• esm: Source Phase Imports for WebAssembly #56919

Electron need to disable source phase imports in renderer and worker processes - Chromium disables them in content/renderer/render_process_impl.cc and the process will hard crash otherwise.

Happy to move this to a different place if there's a better suggestion!

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[targos]

Maybe this could be similar to https://github.com/codebytere/node/blob/3bd7d18f84485e97e87fd056ca2820539b60d647/src/node.cc#L761-L766 and expect --no-js-source-phase-imports to be used?

[targos]

btw it looks like source phase imports don't work at all in node, so I'm not sure why they are enabled by default:

> import.source('./test.mjs')
Promise {
  <pending>,
  Symbol(async_id_symbol): 191,
  Symbol(trigger_async_id_symbol): 6
}
> undefined
Uncaught Error [ERR_VM_MODULE_NOT_MODULE]: Provided module is not an instance of Module
    at importModuleDynamicallyWrapper (node:internal/vm/module:526:41) {
  code: 'ERR_VM_MODULE_NOT_MODULE'
}

> node --no-js-source-phase-imports .\imports.mjs
node:internal/modules/esm/module_job:254
        this.module.instantiate();
                    ^
SyntaxError: Source phase import object is not defined for module 'file:///J:/Projects/test/test.mjs'
    at #_instantiate (node:internal/modules/esm/module_job:254:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:369:5)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:691:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
  code: 'ERR_SOURCE_PHASE_NOT_DEFINED'
}

Node.js v25.0.0

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.58%. Comparing base (2fb82c8) to head (7495bbd).
⚠️ Report is 38 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node.cc	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60364      +/-   ##
==========================================
- Coverage   88.59%   88.58%   -0.01%     
==========================================
  Files         704      704              
  Lines      208474   208401      -73     
  Branches    40067    40054      -13     
==========================================
- Hits       184696   184612      -84     
- Misses      15805    15811       +6     
- Partials     7973     7978       +5     

Files with missing lines	Coverage Δ
src/node.cc	76.03% <75.00%> (-0.04%)	⬇️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codebytere]

Oh lol that's fun - i'm happy to just disable it by default again then? cc @GeoffreyBooth

[joyeecheung]

I think source phase import in Node.js only works with WASM, but not JS.

[targos]

Ok, then I think #60364 (comment) should work

[legendecas]

In order to recognize V8 style negate option --no-js-source-phase-imports, we should declare a --js-source-phase-imports as a V8Option, rather than declaring a standalone --disable....

[legendecas]

Source phase imports now only work with WASM modules. JS source phase imports are waiting on https://github.com/tc39/proposal-esm-phase-imports.

[ljharb]

Indeed, JS source phase imports should be off by default until the feature is stage 3.

[guybedford]

This makes sense. We don't have any snake case flags, perhaps just recase or follow @legendecas's suggestion? Either works for me.

Also I'm pretty sure that this will work with the other codepaths for source phase, but it would be good to check the existing instance phase test to ensure import * as foo from './foo.wasm' still works, by adding this flag to one of those tests in isolation from source phase.

[targos]

My suggestion shouldn't require to add a new option.

[codebytere]

@targos then maybe I'm not clear on what you're suggesting?

[targos]

I'm suggesting to do the same as https://github.com/codebytere/node/blob/3bd7d18f84485e97e87fd056ca2820539b60d647/src/node.cc#L761-L766

Set the flag only if we don't find the --no- prefixed flag in the args.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ljharb]

JS source phase imports should be disabled by default, though - isn't this keeping the opposite?

[legendecas]

JS source phase imports should be disabled by default, though - isn't this keeping the opposite?

The --js prefix in the V8 flag is the new prefix of --harmony. --js-source-phase-imports does not enable https://github.com/tc39/proposal-esm-phase-imports.

[ljharb]

ah, thanks for explaining :-)

[nodejs-github-bot]

Landed in 9bfff20