Skip to content

fix(test): skip --strip-types injection on Node 24+ to unblock vitest#1078

Merged
carlos-alm merged 1 commit into
mainfrom
fix/vitest-node24-strip-types
May 8, 2026
Merged

fix(test): skip --strip-types injection on Node 24+ to unblock vitest#1078
carlos-alm merged 1 commit into
mainfrom
fix/vitest-node24-strip-types

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • Vitest worker forks die at startup on Node 24 with --strip-types is not allowed in NODE_OPTIONS.
  • Node 24 enables type stripping by default and dropped --strip-types from the NODE_OPTIONS allowlist; the config was unconditionally injecting it for major>=23.
  • Gate injection to the Node 22.6+ / Node 23 window (where the flag is required and accepted), and pick --strip-types only on exactly Node 23 — Node 22 still needs --experimental-strip-types.

Why now

Blocks local verification of any other PR on a Node 24 machine. CI is pinned to Node 22 in ci.yml so this hasn't surfaced there, but anyone on a recent Homebrew install hits it immediately.

Test plan

  • npx vitest run tests/builder/detect-changes.test.ts — passes on Node 24.10
  • CI green on Node 22 (regression check that the Node 22 path is unchanged)

Node 24 enables type stripping by default and removed --strip-types from the
NODE_OPTIONS allowlist; vitest worker forks die at startup with "--strip-types
is not allowed in NODE_OPTIONS" because the config unconditionally injected
the flag for major>=23.

Gate the injection to the 22.6+ / 23 window where the flag is both required
and accepted, and pick --strip-types only on exactly Node 23 (Node 22 still
needs --experimental-strip-types).

docs check acknowledged: test-infra-only change; no language, feature,
command, architecture, or roadmap surface affected.
@claude
Copy link
Copy Markdown

claude Bot commented May 8, 2026

Claude finished @carlos-alm's task —— View job


Review Analysis Complete

Todo List:

  • Read and analyze the changed file (vitest.config.ts)
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity assessment
  • Phase 4: Code quality review
  • Phase 5: Backlog compliance evaluation
  • Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Node 24+ crashes Vitest workers because --strip-types was removed from NODE_OPTIONS allowlist, but the config was unconditionally injecting it for Node ≥23. This blocks local development on Node 24 machines.

Why This Approach: The fix correctly identifies that Node 24 has type stripping enabled by default and no longer accepts the flag. The solution gates injection to the specific Node versions that need it (22.6+ and 23 only). This is the right approach - fixing the version detection logic rather than working around symptoms.

Risk Assessment: Low risk. The change is surgical, well-documented, and addresses a legitimate platform compatibility issue. The logic correctly handles edge cases (existing flags, version ranges).

Backlog Compliance

  • Zero-dep: ✓ — No new runtime dependencies added
  • Foundation-aligned: ✓ — Maintains zero-infrastructure principle, fixes local development
  • Problem-fit: 3 — Unblocks development workflow but doesn't directly address core "AI navigation" problem
  • Breaking: No — Internal test configuration change, no API impact
  • Tier: 1

Code Quality Assessment

Strengths:

  • Precise version targeting: vitest.config.ts:9 correctly identifies the exact Node version range where the flag is needed and accepted
  • Flag variant handling: vitest.config.ts:10 properly uses --experimental-strip-types for Node 22 vs --strip-types for Node 23
  • Collision prevention: vitest.config.ts:26 checks for existing flags to avoid duplication
  • Clear documentation: Lines 5-8 explain the Node 24 behavior change and reasoning

Technical Correctness:

  • Version parsing logic is sound (vitest.config.ts:3)
  • Conditional injection prevents both unnecessary flags (Node 24+) and missing flags (Node 22.5-)
  • Environment variable merging preserves existing NODE_OPTIONS

Minor Observations:

  • The logic correctly handles the Node version discontinuity where Node 24 behavior differs from the 22→23 progression
  • Good defensive programming with the filter(Boolean) cleanup

Critical Concerns

None. This is a clean platform compatibility fix that addresses a real blocking issue without compromising code quality or test integrity.

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: Well-implemented fix for a legitimate Node 24 compatibility issue. The solution is precise, well-documented, and low-risk. The only reason for not giving 5 stars is that this is maintenance work rather than exceptional functionality, but it's executed very well.

This PR effectively unblocks local development on Node 24 while maintaining backward compatibility. The version detection logic is correct and the approach is superior to workarounds or version pinning.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Greptile Summary

Gates --strip-types / --experimental-strip-types injection in NODE_OPTIONS to only the Node 22.6–23 window where the flags are both required and accepted, fixing a startup crash in Vitest worker forks on Node 24.

  • Replaces supportsStripTypes (major > 22 || …) with needsStripFlag ((major === 22 && minor >= 6) || major === 23), so Node 24+ never receives the now-banned flag.
  • The stripFlag selector is also updated from major >= 23 to major === 23 for clarity, though functionally redundant given the narrowed injection guard.

Confidence Score: 5/5

Safe to merge — the change is a minimal, well-scoped guard that prevents a startup crash on Node 24 without touching any other behavior.

The old injection condition (major > 22) included Node 24 and beyond, where --strip-types in NODE_OPTIONS is rejected by the runtime. The new needsStripFlag condition precisely covers only the 22.6+/23 range. Node 22 paths are unaffected; Node 24 paths are correctly skipped.

No files require special attention. vitest.config.ts is the only changed file and the fix is straightforward.

Important Files Changed

Filename Overview
vitest.config.ts Narrows strip-types flag injection to the Node 22.6+/23 window only, correctly excluding Node 24+ where the flag is disallowed in NODE_OPTIONS

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Read process.versions.node] --> B{major === 22 AND minor >= 6?}
    B -- Yes --> C[needsStripFlag = true, stripFlag = --experimental-strip-types]
    B -- No --> D{major === 23?}
    D -- Yes --> E[needsStripFlag = true, stripFlag = --strip-types]
    D -- No --> F[needsStripFlag = false, Node lt 22.6 or Node 24+]
    C --> G{Flag already in existing NODE_OPTIONS?}
    E --> G
    G -- No --> H[Inject stripFlag into NODE_OPTIONS]
    G -- Yes --> I[Leave NODE_OPTIONS unchanged]
    F --> I
Loading

Reviews (1): Last reviewed commit: "fix(test): skip --strip-types injection ..." | Re-trigger Greptile

@carlos-alm carlos-alm merged commit 09b7569 into main May 8, 2026
22 checks passed
@carlos-alm carlos-alm deleted the fix/vitest-node24-strip-types branch May 8, 2026 01:30
@github-actions github-actions Bot locked and limited conversation to collaborators May 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant