Skip to content

node: auto-unflag import defer on Node 26.4+ - #770

Open
colinhacks wants to merge 5 commits into
mainfrom
import-defer-unflag
Open

node: auto-unflag import defer on Node 26.4+#770
colinhacks wants to merge 5 commits into
mainfrom
import-defer-unflag

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

import defer now works on Node 26.4+ with no flag or config.

V8's --js-defer-import-eval is refused in NODE_OPTIONS and absent from allowedNodeEnvironmentFlags, so the Unflag shape drops it when the probe succeeds and aborts startup when it does not. Adds Mitigation::UnflagArgv, which rides argv beside v8Flags.

The floor is a Node floor — Node wired the phase through in nodejs/node#63712. Measured: 25.9 rejects the flag, 26.0-26.3 abort in to_phase_constant, 26.4/26.5 defer.

The flag enters V8's flag hash but never NODE_OPTIONS, so #246 cannot recur. Below the floor the error stays a clean SyntaxError. Compat mode and --no-js-defer-import-eval suppress it. Dynamic import.defer() aborts upstream, identically on plain node.

…ation

`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.
…r 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.
Copilot AI lite review requested due to automatic review settings August 19, 2026 16:26
@vercel

vercel Bot commented Aug 19, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Error Error Aug 19, 2026 6:29pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The mechanism is well-reasoned and the matrix row is carefully banded, but two things warrant a decision before merge: the new UnflagArgv shape is the first open-ended band with no removal backstop, and nothing in the diff verifies the flag actually reaches Node's argv.

Reviewed changes — full diff across all 7 files, plus the surrounding flag-injection machinery (compute_inject_flags, unflag_floor, strip_unsupported_node_options), the three spawn call sites, and the existing version_tiers.rs integration harness.

  • New Mitigation::UnflagArgv variant — a V8 flag Node accepts only on the command line, deliberately excluded from unflag_flags_for so it can never enter the NODE_OPTIONS-bound inject set.
  • One matrix rowimport-deferUnflagArgv("--js-defer-import-eval") on the open-ended band [26.4.0, ∞), taking the table from 46 to 47 features.
  • Two new accessorsfeature_matrix::argv_unflag_flags_for and flags::argv_inject_flags, the latter skipping injection when the user already supplied either polarity on argv.
  • Three wiring sitesspawn_node (inside the !compat_mode guard), run_watch, and the compiled-binary launcher.
  • Two unit tests — one pinning the band's floor to the measured Node behaviors, one asserting UnflagArgv flags stay out of the NODE_OPTIONS set.
  • Docs — a new "Deferred module evaluation" section in modern-apis.mdx and an "Unflag on argv" row in the architecture wiki page.

Verified independently and found correct: oxc 0.140 (nub's pinned transpiler) preserves import defer through the TS transform with nub's own options, so the docs' .ts example is not blocked at transpile time; the wiki's countable claims (47 features, twelve Unflag flags, a thirteenth argv-only) all reconcile against the table; and unflag_floor correctly ignores the new variant.

⚠️ The open-ended band has no removal backstop, and structurally cannot reuse the existing one

Every Unflag row with an open-ended band is protected by compute_inject_flags' Stage-4 intersection against the binary's probed process.allowedNodeEnvironmentFlags — the guard the code documents as existing precisely so "no nub release [is] needed when Node removes a flag," with --experimental-permission--permission at Node 24.0 as the cited precedent. argv_inject_flags bypasses that guard, and the probe cannot be retrofitted: Node omits NODE_OPTIONS-refused V8 flags from that set by construction, so it reads false even on the versions where the flag works today.

Because injection is gated on Node version rather than on whether the source uses import defer, the blast radius when V8 eventually drops the flag is every augmented invocation on that Node, not just deferred-import users — and already-shipped nub binaries would abort at startup with no way to pre-empt it.

Technical details
# `UnflagArgv` has no analog to Stage 4's self-correcting probe

## Affected sites
- `crates/nub-core/src/node/feature_matrix.rs:354``band((26, 4, 0), None)`, the first
  open-ended band whose mitigation is not covered by any runtime probe.
- `crates/nub-core/src/node/flags.rs:117-122``argv_inject_flags` takes no
  `accepted_env_flags` parameter, so there is nothing to intersect against and no way to
  simulate a removed flag in a test.
- `crates/nub-core/src/node/spawn.rs:1331`, `crates/nub-cli/src/cli.rs:6761`,
  `crates/nub-launcher/src/main.rs:312` — the three sites that push the result straight
  onto the child's argv.

## Evidence
- V8's `src/flags/flag-definitions.h` documents the lifecycle explicitly: once a shipping
  feature proves stable, "all occurrences of the `FLAG_` variable are removed."
  `--js-defer-import-eval` sits in `JAVASCRIPT_INPROGRESS_FEATURES_BASE`, and
  `proposal-defer-import-eval` is TC39 Stage 3 — so removal is expected eventually, not
  imminent.
- The PR's own evidence string records that Node 25.9 rejects the flag as a hard
  "bad option" startup abort. That is the same failure mode a future flag removal
  produces.
- Nothing downstream can drop it: `strip_unsupported_node_options` only trims inherited
  `NODE_OPTIONS`, and there is no fallback re-spawn.

## Required outcome
A deliberate, recorded decision about what happens to this row when the flag stops
existing — either an automatic guard, or an explicit acceptance that a nub release is
required and the band is manually tracked.

## Suggested approach (optional)
Three shapes, in rough order of cost:
- Cap the band's upper bound at a known-good ceiling and accept manual tracking. Cheapest,
  but forfeits the "no nub release needed" property the rest of the matrix relies on.
- Add an argv-specific probe (`node --js-defer-import-eval -e ""`, or parse
  `node --v8-options`), cached per `(path, mtime)` like `accepted_env_flags` already is.
  Restores self-correction at the cost of one probe spawn per binary.
- Gate injection on the source actually using `import defer`. Shrinks the blast radius
  from "every invocation" to "only invocations that need it", which also addresses the
  inline finding about the dynamic form.

## Open questions for the human
- Is manual tracking acceptable here given `0.0.x`, or is the self-correcting property
  load-bearing enough to pay for a probe?

⚠️ Nothing verifies the flag actually reaches Node's argv

Both new tests exercise argv_unflag_flags_for, which reads the table directly. They assert that the matrix says the right thing — not that any of the three wiring sites exist. Delete all three argv_inject_flags call sites and both tests still pass, while the feature silently does nothing.

This matters more than usual here because the whole feature is the wiring: three separate call sites had to be found and threaded, and a missed one fails silently rather than loudly. The harness for closing this already exists and runs on the exact Node where the feature is live.

Technical details
# The added tests cover the table, not the injection

## Affected sites
- `crates/nub-core/src/node/feature_matrix.rs:1338``import_defer_band_starts_at_the_node_that_works` calls `argv_unflag_flags_for`, one
  function away from the `FEATURES` table it is checking.
- `crates/nub-core/src/node/feature_matrix.rs:1359``argv_only_flags_never_enter_the_node_options_set`; see the inline comment, it guards a
  narrower invariant than its doc comment claims.

## Existing precedent to reuse
`crates/nub-cli/tests/version_tiers.rs:695`
(`module_enabler_flags_make_ffi_vfs_stream_iter_importable`) is the direct analog: it
verifies matrix-driven flag injection end-to-end for the `node:ffi` / `node:vfs` /
`node:stream/iter` rows by running real `nub` against a side-installed **Node 26.5.0** and
asserting on stdout markers. It skips gracefully when that Node is absent.

The infrastructure is already wired:
- `.github/workflows/ci.yml` (~line 900) installs Node 26.5.0 and exports
  `TEST_NODE_BIN_26_5_0`.
- `run_nub_against_node((26, 5, 0), "<fixture>", "<file>")` is the helper.
- Fixtures are a single directory under `tests/fixtures/<name>/`.

## Required outcome
One end-to-end test proving that under nub on Node 26.5, a static `import defer` actually
defers evaluation — i.e. the imported module's side effect does not run until first
property access — and that it fails without the injection.

## Suggested approach (optional)
`tests/fixtures/import-defer/` with a module that prints a marker at top level and an
entry that asserts ordering, plus a `version_tiers.rs` test mirroring
`module_enabler_flags_make_ffi_vfs_stream_iter_importable`. Making the fixture a `.ts`
entry importing a `.ts` module would additionally cover the exact shape the new docs page
advertises, which currently has no coverage at all.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-core/src/node/feature_matrix.rs Outdated
Comment thread crates/nub-core/src/node/feature_matrix.rs Outdated
Comment thread site/content/docs/runtime/modern-apis.mdx Outdated
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.
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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

One recommended change: the new argv probe caches a negative verdict on any non-zero exit, which can silently disable the feature permanently.

Reviewed changes — two new commits since the last review, both responsive to the open threads.

  • 6bc62165 adds the removal backstop that was missing for the open-ended [26.4.0, None) band. discovery::accepts_argv_flag spawns node <flag> -e "" with NODE_OPTIONS cleared and caches the verdict on disk keyed by (path, mtime). This is the right seam — the Stage-4 allowedNodeEnvironmentFlags intersection that protects Unflag rows structurally cannot serve UnflagArgv, and the doc comment explains why. It also adds the end-to-end coverage that was missing: import_defer_actually_defers_evaluation asserts evaluation ordering, not just exit 0, and runs against the side-installed Node 26.5.0 that ci.yml already provisions via TEST_NODE_BIN_26_5_0, so it will actually execute in CI rather than always skipping.
  • ec6fac43 strips nub-injected argv-only flags from process.execArgv in preload-common.cjs, fixing a real ERR_WORKER_INVALID_EXEC_ARGV break. The placement is correct: it runs before any user code and before anything in that file can hand the array out, it only removes flags nub injected (a user's own v8Flags stay visible), and V8 parses these at startup so the feature stays on. It composes cleanly with the pre-existing stripHarmony sanitizer in runtime/worker-polyfill.mjs, which reads process.execArgv after this filtering has already run.

Three verifications worth recording, since they were the load-bearing assumptions:

  • nub watch is safe. Node's --watch supervisor does not run --require preloads (confirmed by pid: only the watched child printed). So the filtering never runs in the supervisor and the flag survives into the re-spawned child — the run_watch wiring works.
  • NODE_OPTIONS flags never appear in process.execArgv, so the only nub-added entries are argv-passed ones and the fixture's execargv:rejected=[] assertion is meaningful rather than vacuous.
  • The custom-V8-snapshot hazard does not reproduce. I had this carried forward as the top open risk — the theory being that --js-defer-import-eval enters V8's flag-list hash and would make SnapshotData::Check() hard-exit on a --snapshot-blob mismatch. Tested both directions on the reachable --build-snapshot / --snapshot-blob path with genuine in-progress harmony flags (--harmony-struct, --harmony-shadow-realm, --js-float16array, --jitless, --no-opt): build-without/load-with and build-with/load-without both loaded cleanly, exit 0. The user-land snapshot path does not enforce the flag hash. Considering this closed. (Probed on Node 24.18, not 26.4+.)

The fork() inheritance question I had flagged is resolved by the docs as written — the Delivery callout already scopes inheritance to subprocesses launched as node through the shim, and fork() launches via absolute process.execPath, so a forked child not receiving the flag matches what the page says.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-core/src/node/discovery.rs
Comment thread crates/nub-launcher/src/main.rs Outdated
`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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — one new commit, 76c2757b, which closes both open threads from the previous review.

  • The probe verdict is narrowed to a genuine rejection (discovery.rs:1298-1301). accepts_argv_flag now returns None unless the non-zero exit carried bad option on stderr, so only a real rejection is persisted to the (path, mtime)-keyed cache and every other failure costs one re-probe rather than the feature. I checked this narrowing is not a silent no-op: in src/node.cc, ProcessGlobalArgsInternal calls V8::SetFlagsFromCommandLine with remove_flags=true, so V8 neither prints nor aborts on its own — the leftover-args loop is the single sink for "unknown to Node and unknown to V8," which is exactly the future flag-removal case this backstop exists for, and StartInternal prints it to stderr. The wording has been stable since Node 6-8 and is identical on Windows.
  • The new unit test can actually fail (discovery.rs:1727-1761). Two fake node shims cover the fork that matters: exit 9 + bad option yields Some(false), exit 1 + unrelated stderr yields None. Run against the pre-commit code the second case returns Some(false), so the test is load-bearing rather than ceremonial. unique_tmp gives each run a pid+nanos-unique dir, so the real cache file it writes cannot collide between tests.
  • argv_inject_flags now takes Option<&Path>, with None preserving pure version-band behavior. All three call sites are updated (spawn.rs:1331, cli.rs:6761, and the launcher) — grep confirms that is the complete set.
  • The launcher skips the probe for NodeOrigin::Managed (main.rs:315-319), matching the accepted_env_flags skip twelve lines up.

On that last one: the skip does mean a Managed Node whose baked-in version sits above a band floor after V8 removes the flag would keep receiving it. That exposure is real — nub compile resolves the Node version live against the dist index, so it can outrun the compiling binary's own feature table — but it is precisely the exposure the pre-existing accepted_env_flags skip already accepts for the entire NODE_OPTIONS inject set, and seven open-ended (lo, None) bands ship through that same path today (--experimental-vm-modules, --experimental-ffi, --experimental-vfs, and others). Recovery is identical in both cases — recompile with an updated nub — and a compiled artifact has no runtime escape hatch for either. Consistent with the rest of the tree, too: nub-core's spawn path probes unconditionally for both flag shapes; the origin gate exists only in the launcher's hot path. Nothing here is worse than what is already merged, so I'm not raising it.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants