fix(build): gate native typeof window folding - #2574
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: gate native typeof window folding
Reviewed the version-gating logic, the plugin apply/transform changes, and the new fixtures. The core approach is sound and correctly restores pre-#2564 behavior for older Vite while keeping native folding on 8.1.4+. Verified vp check (tsc), and tests/tsconfig-paths-vite8.test.ts + tests/type-of-window.test.ts pass locally (21/21).
Correctness — looks good
- Boundary math is right.
major > 8 || (major === 8 && (minor > 1 || (minor === 1 && patch >= 4)))evaluates correctly at the fixture boundaries:8.1.3→ false,8.1.4→ true,8.2.0/9.0.0→ true. - Dev parity restored for old Vite. Dropping
apply: "build"in favor ofapply(_config, env) => !useNativeTypeofWindowFolding || env.command === "build"makes the scan transform run in serve too on pre-8.1.4. This matches the pre-#2564 behavior (before native folding, the transform had noapplyrestriction and ran in dev + build), so this is a faithful restoration, not a new behavior.env.commandis the correct property (consistent with the existinglimitToCommandhelper atpackages/vinext/src/index.ts:1534). bundledVersionsfallback is exercised in this repo.vite/package.jsonhere is@voidzero-dev/vite-plus-corewithbundledVersions.vite: "8.1.2", sogetViteVersionFromRequirecorrectly falls through to the bundled version.
Heads-up: this repo currently takes the custom-transform path
Because the bundled Vite is 8.1.2 (< 8.1.4), supportsNativeTypeofWindowFolding is false in this repo, CI, and all deploy previews/benchmarks. That means this PR effectively reverts the #2564 native-folding perf win for the shipping environment until Vite reaches 8.1.4+. This is consistent with the PR's stated rationale (8.1.4 is the first release with the required Rolldown update) and is the likely cause of the +7.8% production build time regression reported by the benchmark bot — the AST-based scope-aware transform is slower than native Oxc define folding. Flagging so it's a conscious, documented tradeoff rather than a surprise; it should recover automatically once the bundled Vite crosses 8.1.4.
Non-blocking nits
-
majorfield is dead.assertSupportedViteVersion()now returns{ major, supportsNativeTypeofWindowFolding }, but no caller readsmajor(packages/vinext/src/index.ts:1298only destructuressupportsNativeTypeofWindowFolding). Consider droppingmajorfrom the return type to keep the surface minimal, or keep it if you expect near-term consumers. -
Test re-implements the gating logic instead of importing it (
tests/type-of-window.test.ts:43-45). Duplicating themajor/minor/patch >= 4expression risks source/test drift, and the test'sviteVersion.split(".").map(Number)diverges from the source's regex^(\d+)\.(\d+)\.(\d+)for prerelease strings: e.g.8.1.4-beta.1yieldspatch = Number("4-beta") = NaNin the test (→false) vs4via the source regex (→true). Bundled versions are clean today so it passes, but importing the parser/predicate fromutils/vite-version.tswould remove both the drift risk and the parsing mismatch.
Overall: correct and well-tested. The only substantive item is awareness of the perf regression while the bundled Vite is < 8.1.4.
|
Review posted to PR #2574. Summary of my findings: Verdict: Correct and well-tested; one perf awareness itemCorrectness is solid:
Key heads-up (not a bug): Because the bundled Vite is 8.1.2 < 8.1.4, this repo/CI/previews take the custom transform path, so the PR reverts the #2564 native-folding perf win for now — which is almost certainly the cause of the benchmark bot's +7.8% production build time regression. This is consistent with the PR's rationale and should self-recover once Vite ≥ 8.1.4. Non-blocking nits:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: gate native `typeof window` folding
Reviewed HEAD (`953e6a8`), which includes the follow-up commit addressing both nits from the prior review. Verified `vp check` (fmt + type-aware lint, clean) and ran `tests/type-of-window.test.ts` + `tests/tsconfig-paths-vite8.test.ts` (22/22 pass).
Both prior nits are now resolved
-
Dead `major` field removed. `assertSupportedViteVersion()` now returns only `{ supportsNativeTypeofWindowFolding }` (
utils/vite-version.ts:120-131), and `major` is used internally just for the `< 8` guard. Clean. -
Test now imports the source predicate. `type-of-window.test.ts:15,44` imports `supportsNativeTypeofWindowFolding` from `utils/vite-version.ts` instead of re-implementing the version comparison, eliminating the drift risk. The new prerelease/build-metadata cases (`8.1.4-beta.1` → false, `8.1.4+build.1` → true, `8.2.0-beta.1` → true) are correct.
Correctness — confirmed
-
Boundary math verified exhaustively. Ran the predicate against 16 cases including prerelease and build-metadata edges (`8.1.3`→false, `8.1.4`→true, `8.1.4-beta.1`→false, `8.1.4+build.1`→true, `8.1.40`→true, `9.0.0-beta.1`→true, garbage→false). All correct. The regex `^(\d+).(\d+).(\d+)(-…)?(?:+…)?$` now parses prerelease/build tags properly, and prerelease on the exact `8.1.4` boundary correctly resolves to false while prerelease on any higher version resolves to true.
-
Faithful restoration of pre-#2564 behavior. Confirmed against `b6571ce^`: before native folding there was a single unrestricted `vinext:typeof-window` transform running in both serve and build. For Vite < 8.1.4 this PR reproduces that: `configEnvironment` returns `null` (no native define) and the scan plugin's `apply` returns `true` for both serve and build with the handler no longer early-returning on `write !== false`. Not new behavior.
-
`apply(_config, environment)` uses the correct `command` property, consistent with the existing `limitToCommand` helper (
index.ts:1534). -
`bundledVersions.vite` fallback exercised in this repo (`vite/package.json` is `@voidzero-dev/vite-plus-core` with bundled `8.1.2`).
Perf tradeoff (unchanged, non-blocking)
Because the bundled Vite is `8.1.2` (< 8.1.4), this repo/CI/previews take the custom-transform path, so the #2564 native-folding win is temporarily reverted for the shipping environment. This is the likely cause of the benchmark bot's +7.8% production build time and is consistent with the PR's rationale — it should self-recover once bundled Vite reaches ≥ 8.1.4. Deliberate, documented tradeoff.
Verdict
LGTM. Both review items from the prior pass are addressed, correctness is verified at the version boundary and in the plugin wiring, and the only remaining item is the intentional, self-healing perf tradeoff.
|
Review posted to PR #2574. SummaryThe PR gates native Verdict: LGTM. No blocking issues found. Key findings:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: gate native typeof window folding (HEAD a833879)
Reviewed the follow-up commit a833879 ("detect bundled Rolldown folding support"), which landed after the two prior review rounds and materially changes the earlier conclusions. Verified vp check (fmt + type-aware lint, clean) and ran tests/type-of-window.test.ts + tests/tsconfig-paths-vite8.test.ts (24/24 pass).
What changed since the last review
The new commit makes supportsNativeTypeofWindowFolding accept an optional bundled Rolldown version and take the native path when either the Vite version is ≥ 8.1.4 or the bundled Rolldown is ≥ 1.1.4. getViteToolchainVersion now returns { vite, rolldown }, reading bundledVersions.rolldown from npm-alias packages.
Important correction to the prior reviews' "perf regression" heads-up: this repo's vite/package.json is @voidzero-dev/vite-plus-core@0.2.2 with bundledVersions: { vite: "8.1.2", rolldown: "1.1.4" } (confirmed on disk). Because bundled Rolldown is 1.1.4, this commit flips the repo/CI/previews back onto the native folding path. So the +7.8% build-time regression the earlier rounds flagged as "expected/self-healing" should actually be resolved by this commit — the shipping environment no longer takes the slower custom-transform path. Worth re-running the benchmark bot to confirm.
Correctness — verified
- Boundary math in
isVersionAtLeastis right at all edges (8.1.3→false,8.1.4→true, prerelease on the exact boundary→false, prerelease above→true, build-metadata→true). Rolldown cases (1.1.3→false,1.1.4-beta.1→false,1.1.4→true) verified by the new test attests/type-of-window.test.ts:63-65. - Real-vite path (
name === "vite") returns{ vite }with no rolldown, so it correctly falls back to the pure Vite-version check. - Plugin wiring (
configEnvironmentreturningnullon the custom path, scanapply=!native || command === "build", handler early-return gated onnative && write !== false) faithfully restores pre-#2564 behavior for old toolchains while keeping native folding + write-less analysis folding for new ones. - Non-null assertion at
utils/vite-version.ts:148is safe (only parseable versions escapegetViteToolchainVersion).
Nit (non-blocking): Rolldown check can be overridden by a newer bundled Vite
supportsNativeTypeofWindowFolding at utils/vite-version.ts:121-127:
if (bundledRolldownVersion && isVersionAtLeast(bundledRolldownVersion, 1, 1, 4)) return true;
return isVersionAtLeast(viteVersion, 8, 1, 4);For an npm-alias package, viteVersion is bundledVersions.vite. If an alias ever reports bundledVersions: { vite: "8.2.0", rolldown: "1.1.3" } (bundled Vite ≥ 8.1.4 but bundled Rolldown < 1.1.4), the function returns true via the Vite check even though the actual capability gate (per the PR's own framing) is the Rolldown version. Since the PR treats Rolldown 1.1.4 as the authoritative capability signal, when a bundled Rolldown version is present it should arguably be authoritative rather than being overridable by the bundled Vite number.
This is almost certainly impossible in practice (Vite 8.1.4 pins Rolldown 1.1.4), so it's a robustness/consistency point, not a live bug. All alias fixtures in tests/tsconfig-paths-vite8.test.ts use bundled Vite 8.1.2, so this new-vite/old-rolldown combination is untested. If you want to close the gap, add a fixture with { vite: "8.2.0", rolldown: "1.1.3" } and decide the intended outcome (I'd expect false).
Verdict
LGTM. The follow-up commit is correct, well-tested at the version boundaries, and — notably — should reverse the build-time regression the earlier reviews attributed to this PR, since the repo's bundled Rolldown (1.1.4) now qualifies for native folding. The only open item is the minor consistency nit above.
|
Review posted to PR #2574. SummaryI reviewed the PR at its actual HEAD ( Verdict: LGTM — correct, well-tested, with one non-blocking nit. Key findings:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: gate native typeof window folding (HEAD 0f1f1f6)
Reviewed the current HEAD, which includes 0f1f1f6 ("test(build): prefer bundled Rolldown capability") — a commit that landed after all three prior review rounds and closes the last open nit. Verified vp check (fmt + type-aware lint, clean) and ran tests/type-of-window.test.ts + tests/tsconfig-paths-vite8.test.ts (24/24 pass).
Verdict: LGTM. No blocking issues, and the one remaining consistency nit from the prior round is now resolved.
The last open nit is fixed
The prior review flagged that supportsNativeTypeofWindowFolding let a newer bundled Vite override an older bundled Rolldown (vite: 8.2.0 + rolldown: 1.1.3 → true). At utils/vite-version.ts:121-128 the logic now makes Rolldown authoritative whenever it is present:
if (bundledRolldownVersion !== undefined) {
return isVersionAtLeast(bundledRolldownVersion, 1, 1, 4);
}
return isVersionAtLeast(viteVersion, 8, 1, 4);So 8.2.0 + rolldown 1.1.3 now correctly returns false. This is covered by a new unit case (type-of-window.test.ts:66) and the intended-outcome question from the prior round is answered: bundled Rolldown wins.
Correctness — verified
- Boundary math in
isVersionAtLeastis right at all edges: Vite8.1.3→false,8.1.4→true, prerelease on the exact boundary (8.1.4-beta.1)→false, prerelease above (8.2.0-beta.1)→true, build-metadata (8.1.4+build.1)→true. Rolldown1.1.3→false,1.1.4-beta.1→false,1.1.4→true. - Rolldown-authoritative fallback is safe.
getViteToolchainVersionFromRequireonly setsrolldownwhen it parses cleanly (vite-version.ts:99), so a missing/garbagebundledVersions.rolldownfalls through to the Vite check rather than short-circuiting. - Real-vite path (
name === "vite",vite-version.ts:90-92) returns{ vite }with no rolldown, correctly deferring to the pure Vite-version check for real Vite installs. - This repo takes the native path. On-disk
vite/package.jsonis@voidzero-dev/vite-plus-core@0.2.2withbundledVersions: { vite: "8.1.2", rolldown: "1.1.4" }; bundled Rolldown 1.1.4 qualifies, so CI/previews use native folding — the earlier "+7.8% build-time regression" heads-up is not applicable at HEAD. - Faithful pre-#2564 restoration for old toolchains. Confirmed against
b6571ce^: the original was a single unrestrictedvinext:typeof-windowtransform running in serve+build with nowriteearly-return. On the custom path this PR reproduces that —configEnvironmentreturnsnull(no native define), scanapply=!native || command === "build", handler early-return gated onnative && write !== false. apply(_config, environment)readsenvironment.command, consistent with the existinglimitToCommandhelper (index.ts:1534-1535).- Non-null assertion at
vite-version.ts:150is safe — only versions that already parsed cleanly (vite-version.ts:90/95) escapegetViteToolchainVersion.
Nit (non-blocking, cosmetic)
The apply(_config, environment) param at index.ts:5439 names the ConfigEnv ({ command, mode }) environment, which reads like a Vite Environment. env (as at index.ts:1534) would be less ambiguous. No behavioral impact.
The test coverage matrix is thorough: real Vite 8.1.3/8.1.4, alias with bundled Rolldown 1.1.4, and alias with bundled Rolldown 1.1.3 (tsconfig-paths-vite8.test.ts), plus the version-predicate unit cases. Correct and well-tested.
|
Review posted to PR #2574. SummaryI reviewed the current HEAD ( Verdict: LGTM — no blocking issues. Key findings:
Only remaining item is a cosmetic naming nit ( |
1 participant
Summary
typeof windowdefine folding on Vite 8.1.4 or newerRegular Vite first receives the required Rolldown update in Vite 8.1.4. The workspace currently aliases Vite to Vite+ 0.2.2, which reports bundled Vite 8.1.2 but already reports bundled Rolldown 1.1.4, so it can safely keep the native fast path.
Validation
vp checkvp test run tests/tsconfig-paths-vite8.test.ts tests/type-of-window.test.tsvp test run tests/app-browser-server-action-client.test.ts tests/app-server-action-execution.test.ts