[james-elicx]

Summary

• configure native per-environment typeof window defines so real client/server builds use Oxc/Rolldown folding
• retain vinext's scope-aware fold only for plugin-RSC's write-less analysis builds, where the lexer replaces source before native define folding runs
• remove the broad custom transform and its mutable cache-directory hook filter

Performance

The custom AST fold no longer runs for normal production modules. It is limited to plugin-RSC's two analysis builds and is invoked only for modules matching the native typeof window hook filter.

Same-worktree hyperfine A/B on the action fixture showed no regression; the analysis fold trended 5-8% faster because dead imports were omitted from the synthetic scan graph.

Validation

• pnpm exec vitest run tests/type-of-window.test.ts
• 10 tests passed

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2564

npm i https://pkg.pr.new/create-vinext-app@2564

npm i https://pkg.pr.new/vinext@2564

commit: 09b04d4

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[github-actions]

Performance benchmarks

Compared 09b04d4 against base aa1b666 using alternating same-runner rounds. Next.js was unchanged and skipped.

1 improved · 0 regressed · 5 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	126.6 KB	126.6 KB	⚫ 0.0%
Client entry size (gzip)	vinext	120.6 KB	120.6 KB	⚫ +0.0%
Dev server cold start	vinext	2.73 s	2.73 s	⚫ -0.2%
Production build time	vinext	3.07 s	2.85 s	🟢 -7.2%
RSC entry closure size (gzip)	vinext	98.5 KB	98.5 KB	⚫ -0.0%
Server bundle size (gzip)	vinext	165.1 KB	165.1 KB	⚫ -0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: perf(build): use native typeof window folding

Reviewed the change end-to-end (plugin-RSC scan-build internals, native define wiring, dev/prod parity, and test coverage). This is a clean, well-reasoned change. No blocking issues.

Verified correct

• Scan-build gating is accurate. plugin-RSC sets build.write = false on the rsc/ssr environments during its two analysis passes (@vitejs/plugin-rsc plugin-BK29Va7z.js:640-641) and restores true before the real builds (647-648). The new if (this.environment.config.build.write !== false) return null; correctly scopes the custom AST fold to exactly those scan passes.
• The scan-strip rationale holds. scanBuildStripPlugin (enforce: "post") rewrites each module to just its lexer-discovered imports before Rolldown's native define folding runs, so typeof window is gone by then — which is why the scope-aware fold is still needed there to drop dead browser/server imports from the synthetic graph.
• Native define maps consumers correctly. configEnvironment gives client → "object" and everything else (server/rsc/ssr, consumer === "server") → "undefined". Confirmed by the new define assertion test and, importantly, by the retained end-to-end buildApp() test (my-differentiated-files absent from dist/server/ssr, BROWSER present in dist/client).
• Dev parity holds. tests/server-globals.test.ts ("clears browser globals ... in dev") still passes — native define folds typeof window → "undefined" in the dev SSR environment, so the fixture's leak guard doesn't throw.
• No dangling references / clean removal. getCacheDirPrefix and typeofWindowIdFilter fully removed; the escapeRegExp import is correctly dropped (still used in other modules); toSlash still imported. vp check (lint + types) clean; all 10 type-of-window tests pass.
• Filter refinement (\btypeof\s+window\b) is safe — it's narrower than the internal /typeof\s+window/, but only excludes non-boundary cases like typeof windowXY that shouldn't fold anyway.

Minor, non-blocking notes

1. Consider apply: "build" on vinext:typeof-window-scan. It's functionally correct today (dev returns early since build.write is undefined, not false), but adding apply: "build" would stop the code filter from running on every dev module containing typeof window, matching the sibling vinext:remove-console plugin.

2. Behavioral change worth flagging in the description. Dev and real production builds now use token-based native define (not scope-aware), so a local window binding would be folded. This actually matches Next.js's DefinePlugin/SWC behavior, so it's a parity improvement rather than a regression — but it's a deliberate semantic shift away from the previous scope-aware dev/real-build folding and worth calling out.

3. Lost coverage for the cacheDir guard. The two deleted tests exercised the cacheDir-exclusion logic, which still lives in the scan handler (index.ts:5441-5442) but is now untested. Either add a focused test that the scan fold skips cacheDir ids, or confirm the guard is unreachable in write-less scan builds and drop it.

Nice result on the -8.2% production build time. LGTM.

[ask-bonk]

Review posted to PR #2564.

Summary

I reviewed PR #2564 (perf(build): use native typeof window folding) end-to-end and found no blocking issues. The change is sound.

Key verifications:

• Confirmed in the @vitejs/plugin-rsc source that scan builds set build.write = false and restore true for real builds, so the new build.write !== false gate correctly limits the custom AST fold to scan passes.
• Confirmed the scan-strip rationale (module bodies replaced with lexer imports before native define runs) is accurate.
• Confirmed native define maps consumers correctly and that the retained end-to-end buildApp() test proves it works (browser import stripped from SSR, present in client).
• Confirmed dev parity by running tests/server-globals.test.ts — dev SSR still folds typeof window correctly via native define.
• Confirmed clean removal (no dangling refs, escapeRegExp still used elsewhere), and vp check + all 10 tests pass.

Non-blocking notes raised:

1. Consider adding apply: "build" to the scan plugin to avoid running its filter in dev.
2. The switch to token-based native define (not scope-aware) is a deliberate behavioral change that actually improves Next.js parity — worth noting in the description.
3. The two deleted tests removed coverage of the cacheDir guard that still exists in the scan handler — either re-test it or confirm it's dead code and remove it.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bonk review the latest PR head for issues

[ask-bonk]

APIError: Function tools with reasoning_effort are not supported for gpt-5.5 in /v1/chat/completions. Please use /v1/responses instead.

github run

[james-elicx]

/bigbonk review for issues, scoped to the latest commit 09b04d4 only (the apply: build addition and cacheDir regression test). Post a concise blocker/no-blocker verdict.

[ask-bonk]

Review: latest commit `09b04d4` only (test(build): tighten typeof window scan plugin)

Scoped strictly to this commit — the `apply: "build"` addition and the cacheDir regression test.

Verdict: no blocker. ✅

`apply: "build"` on `vinext:typeof-window-scan`

• The pre-existing `build.write !== false` guard already made the handler a no-op in dev (dev has `build.write === undefined`), so this is not a behavior change for builds. It correctly stops the `{ code: /\btypeof\s+window\b/ }` filter from running on every dev module. This addresses non-blocking note Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1 from the prior review and matches the established pattern used by 17 other plugins in `index.ts` (including the sibling define plugin at :5399). ✓

cacheDir regression test

• Directly addresses the prior review's non-blocking note fix: exclude vinext from optimizeDeps to prevent virtual module resolution errors #3 (lost cacheDir-guard coverage).
• Good design choices verified against the handler (`index.ts:5441-5443`):
  • cacheDir uses a regex-special substring `.vite-cache[custom]`, proving the guard relies on `toSlash(id).startsWith(cacheDir)` string matching, not regex. ✓
  • Mock context (`build.write: false`, `cacheDir`, `consumer: "server"`) accurately mirrors the runtime shape consumed by the handler and `getTypeofWindowReplacement` (`plugins/typeof-window.ts:70`). ✓
  • Both assertions exercise real paths: cacheDir id → early-return `null`; app id → genuine fold (server ⇒ `"undefined"`, `!==` test false ⇒ branch removed ⇒ non-null), not a filter bypass. ✓

Verification

• `vitest run tests/type-of-window.test.ts` → 11 passed.
• `vp check` on both changed files → clean (format, lint, types).

Minor, non-blocking

• The test invokes `transform.handler` directly, so it doesn't cover the `filter` regex itself; a filter regression wouldn't be caught here. Acceptable scope for a handler unit test.

LGTM for this commit.

[ask-bonk]

Reviewed the latest commit 09b04d4 in isolation and posted the verdict to PR #2564.

Verdict: no blocker. ✅

The commit does exactly two things, both sound:

1. apply: "build" on vinext:typeof-window-scan — The existing build.write !== false guard already made the handler a no-op in dev, so this changes no build behavior; it just stops the code filter regex from running on every dev module. Consistent with 17 other plugins in index.ts and addresses the prior review's note Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1.

2. cacheDir regression test — Addresses the prior review's note fix: exclude vinext from optimizeDeps to prevent virtual module resolution errors #3 (lost cacheDir coverage). Well-constructed: uses a regex-special cacheDir substring ([custom]) to prove startsWith matching, mock context accurately mirrors the runtime shape, and both assertions exercise real code paths (null early-return vs. genuine fold), not filter bypasses.

Verified: vitest run tests/type-of-window.test.ts → 11 passed; vp check on both files → clean. One minor non-blocking note: the test calls transform.handler directly, so the filter regex itself isn't covered — acceptable for a handler-level unit test.

github run