module: enable existing machinery for deferred imports - #63712
Merged
Conversation
Collaborator
|
Review requested:
|
MayaLekova
changed the title
nodejs-github-bot
added
c++
MayaLekova
force-pushed
the
import-defer-01
branch
from
June 2, 2026 14:22
0968bc3 to
6bb2db9
Compare
joyeecheung
reviewed
Jun 2, 2026
joyeecheung
left a comment
joyeecheung
left a comment
Member
There was a problem hiding this comment.
I think the commit message needs to clarify that this only makes it work for static import, but not dynamic import yet?
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63712 +/- ##
========================================
Coverage 90.34% 90.35%
========================================
Files 732 732
Lines 236580 236709 +129
Branches 44545 44585 +40
========================================
+ Hits 213748 213870 +122
- Misses 14536 14565 +29
+ Partials 8296 8274 -22
π New features to boost your workflow:
|
MayaLekova
force-pushed
the
import-defer-01
branch
from
June 3, 2026 12:56
6bb2db9 to
5e52c9a
Compare
Contributor
Author
|
Thank you, I moved the test, but should I add more extensive test coverage, even for static imports? |
Member
|
I think a smoke test is enough. The rest should be covered by test262 in the upstream. Can you rewrap the commit message to 72 chars to make the linter happy? Also this needs adding a eslint plugin
|
marco-ippolito
approved these changes
Jun 5, 2026
MayaLekova
force-pushed
the
import-defer-01
branch
3 times, most recently
from
June 8, 2026 15:08
2915765 to
bd9968c
Compare
guybedford
reviewed
Jun 8, 2026
MayaLekova
force-pushed
the
import-defer-01
branch
from
June 9, 2026 16:35
bd9968c to
569d71c
Compare
guybedford
approved these changes
Jun 9, 2026
guybedford
left a comment
guybedford
left a comment
Contributor
There was a problem hiding this comment.
Works perfectly thank you!
joyeecheung
approved these changes
Jun 9, 2026
github-actions
Bot
removed
the
request-ci
Collaborator
joyeecheung
reviewed
Jun 9, 2026
MayaLekova
force-pushed
the
import-defer-01
branch
from
June 10, 2026 13:01
569d71c to
a519846
Compare
Contributor
Author
|
Thanks for the reviews! Do you mind checking the updated version? (minor changes in tests only) |
guybedford
approved these changes
Jun 10, 2026
guybedford
left a comment
guybedford
left a comment
Contributor
There was a problem hiding this comment.
Test looks good. It would be great to see a Wasm test too (import defer * as foo from './foo.wasm'), but that also isn't necessary either.
github-actions
Bot
removed
the
request-ci
Collaborator
gurgunday
approved these changes
Jun 11, 2026
joyeecheung
approved these changes
Jun 11, 2026
This commit enables deferred import for statically imported modules, by using the corresponding functionality in V8. It adds two tests: - smoke test exercising the basic syntax - test for deferred import of module with its own imports More test coverage should be added for mixing ESM and CJS modules, as well as TypeScript modules. Refs: https://github.com/tc39/proposal-defer-import-eval Signed-off-by: Maya Lekova <maya@igalia.com>
MayaLekova
force-pushed
the
import-defer-01
branch
from
June 12, 2026 14:50
a519846 to
7b9767e
Compare
joyeecheung
approved these changes
Jun 13, 2026
github-actions
Bot
removed
the
request-ci
Collaborator
Collaborator
Collaborator
Collaborator
guybedford
added
author ready
nodejs-github-bot
removed
the
commit-queue
Collaborator
|
Landed in b09155d |
Contributor
Author
Thank you for helping me land this PR! I'll add more tests in the next commits, including some with Wasm modules. |
aduh95
pushed a commit
that referenced
this pull request
Jun 18, 2026
This commit enables deferred import for statically imported modules, by using the corresponding functionality in V8. It adds two tests: - smoke test exercising the basic syntax - test for deferred import of module with its own imports More test coverage should be added for mixing ESM and CJS modules, as well as TypeScript modules. Refs: https://github.com/tc39/proposal-defer-import-eval Signed-off-by: Maya Lekova <maya@igalia.com> PR-URL: #63712 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: GΓΌrgΓΌn DayΔ±oΔlu <hey@gurgun.day>
aduh95
added
the
backport-requested-v24.x
Contributor
|
This doesn't land cleanly on v24.x-staging, it would require a manual backport PR if we want it on Node.js 24 |
colinhacks
added a commit
to nubjs/nub
that referenced
this pull request
Aug 20, 2026
* node: auto-unflag `import defer` on Node 26.4+ via an argv-only mitigation `import defer * as ns from "m"` needs V8's `--js-defer-import-eval`. Node refuses that flag in NODE_OPTIONS in both polarities and omits it from `process.allowedNodeEnvironmentFlags`, so it cannot use the existing `Unflag` shape: `compute_inject_flags` would drop it in the Stage-4 accepted-flag intersection whenever that probe succeeds, and would feed it to the NODE_OPTIONS-bound script-runner path whenever the probe fails. Add a `Mitigation::UnflagArgv` shape for a V8 flag Node accepts only on the command line, plus an `import-defer` row banded to 26.4+. The floor is a Node floor, not a V8-flag one: V8 has carried the flag since 26.0, but Node only wired the defer phase through in nodejs/node#63712, first released in 26.4.0. Verified against real binaries β 25.9 rejects the flag outright, 26.0-26.3 accept it and then abort in to_phase_constant, 26.4 and 26.5 defer correctly. Injection happens at the argv-bearing call sites (spawn, watch, the compiled launcher) alongside user `v8Flags`, never through NODE_OPTIONS. That is what keeps it clear of the Electron snapshot-hash crash in #246: the flag does enter V8's flag hash, but an argv flag reaches only the process nub spawns, never a downstream Electron binary spawned by absolute path. * docs: note that an absolute-path subprocess does not inherit the defer flag Argv delivery does not propagate to a child spawned through Node's own executable path. A child launched as `node` still gets it through the shim. Verified with a spawnSync fixture on Node 26.5. * node: add a removal backstop and an end-to-end test for the defer unflag Addresses four review findings on #770. An open-ended `UnflagArgv` band had no analog to `compute_inject_flags`' Stage-4 intersection, and could not reuse it: a command-line-only V8 flag is absent from `process.allowedNodeEnvironmentFlags` by construction, so that probe reads false even where the flag works. Without a backstop, V8 eventually removing the flag would abort EVERY augmented invocation on that Node β an unknown `--js-*` is a hard "bad option", verified β including from binaries already shipped. Add `discovery::accepts_argv_flag`, which spawns the binary once per (binary, flag) and caches the verdict by (path, mtime); `argv_inject_flags` now intersects against it. Below the band's floor nothing is probed, so an out-of-band Node pays no extra spawn. Verified by reporting the flag as unaccepted: the injection is dropped and the program fails with a clean SyntaxError instead of aborting. Record the dynamic form as an ACCEPTED ADDITIVITY EXCEPTION rather than charging it upstream. Measured against the real user baseline (`node app.js`, no flag), bare Node raises a catchable SyntaxError on `import.defer(spec)` while nub dies on an uncatchable V8 fatal error. Saying it "reproduces on plain Node" was true only of `node --js-defer-import-eval`, which no user runs. The docs callout now states that delta plainly. Strengthen the NODE_OPTIONS-leak test to assert against `compute_inject_flags`, whose output actually becomes that payload. Comparing the two matrix accessors was near-vacuous β they match disjoint variants β and missed routes that bypass the table, `ALWAYS_INJECT` above all. Add `import_defer_actually_defers_evaluation`, the only test covering the wiring rather than the table: the previous tests would all pass with every `argv_inject_flags` call site deleted. It asserts ORDERING, so a non-deferring implementation fails, and uses a .ts entry importing a .ts module β the shape the docs advertise, previously uncovered. * node: hide nub's argv-only V8 flags from process.execArgv Fixes a real regression this branch introduced: a Next.js 16 + Turbopack build died under nub on Node 26.4+ with ERR_WORKER_INVALID_EXEC_ARGV: Initiated Worker with invalid NODE_OPTIONS env variable: --js-defer-import-eval is not allowed in NODE_OPTIONS nub injects that flag on argv precisely BECAUSE Node refuses it in NODE_OPTIONS, but the flag still lands in `process.execArgv` β and a great deal of real tooling forwards `process.execArgv` into a Worker or into a child's NODE_OPTIONS. Node then rejects nub's own flag and kills the build. Every other flag nub injects is NODE_OPTIONS-legal, so this hazard is specific to the new UnflagArgv shape. The spawn, watch and compiled-launcher paths now pass the injected argv-only flags to the preload through `__NUB_ARGV_ONLY_FLAGS`, and the preload removes them from `process.execArgv` before any user code runs, then deletes the variable. V8 parses these flags at startup, so the feature stays on: verified that `import defer` still defers on the main thread AND inside a worker that received the filtered execArgv. Only flags NUB injected are hidden; a user's own `v8Flags` stay visible. This restores the execArgv a plain-Node user would have seen, rather than changing Node behavior β the same reasoning as worker-polyfill.mjs stripping `--harmony-*` for the identical error. Covered by `injected_argv_only_flags_are_hidden_from_exec_argv`, which asserts both halves: nothing left in execArgv is NODE_OPTIONS-illegal, and deferral still works inside a worker handed that execArgv. The bug escaped the earlier round because the manual worker check used a bare `new Worker(...)` with no execArgv option. * node: only cache a genuine rejection from the argv-flag probe `accepts_argv_flag` treated every non-zero exit as "Node rejected this flag" and persisted that verdict by (path, mtime). One transient failure β a signal under memory pressure, a sandbox denial, a half-written install β would therefore have disabled the feature for that binary until Node was reinstalled or the cache file deleted by hand, with no diagnostic. Its sibling `accepted_env_flags` returns `None` on a failed probe and writes nothing, so a transient failure there costs a single re-probe. The false negative was also the LIKELY case rather than the rare one: this probe only runs at or above a band floor, where the flag is expected to work, while the rejection it exists to catch is a future flag removal. Narrow the durable verdict to the exact signal, measured on real binaries: Node 25.9.0 rejects with exit 9 and `node: bad option: --js-defer-import-eval` on stderr, and 26.5.0 accepts with exit 0 and empty stderr. Anything else returns `None`, falling back to version-band gating. Covered by `argv_flag_probe_caches_a_rejection_but_not_a_transient_failure`, which drives both paths through fake binaries. Also stop probing a Node that nub provisioned or embedded itself: `argv_inject_flags` now takes an optional path, and the compiled launcher passes `None` for a managed Node. That mirrors why `accepted_env_flags` is already skipped there β its accepted flags follow from its version, and the launcher is the hot path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
This commit enables deferred module imports by using the corresponding functionality in V8. It also adds a single test, but more test coverage should be added.
Refs: https://github.com/tc39/proposal-defer-import-eval