fix(dev): refresh Pages hydration after render errors - #2823
Conversation
commit: |
|
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
953e309 to
7cbe49a
Compare
7cbe49a to
31bb9b4
Compare
|
/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: fix(dev): refresh Pages hydration after render errors
I reviewed the diff against the actual Vite (@voidzero-dev/vite-plus-core) HTML transform pipeline to confirm the mechanism. The fix is correct and addresses a real bug, but I found one substantive issue plus a couple of minor notes.
Root cause confirmed (fix is sound)
Vite's dev HTML proxy cache is htmlProxyMap: WeakMap<config, Map<filePath, result[]>>, and addToHTMLProxyCache overwrites map.get(filePath)[index] on every transform of the same document URL (node.js addToHTMLProxyCache). So a slow error render and a fast success render to the same clean URL clobber each other's inline-proxy contents while the stable ?html-proxy&index=N.js URL is reused — producing stale/mismatched hydration. Capturing the exact proxy source, content-addressing it into an immutable __vinext_html_proxy_content_<hash>_<index>.js URL, and serializing same-URL transforms with a per-document lock is the right fix.
Hook ordering also checks out: in dev, Vite runs preHooks → htmlEnvHook → devHtmlHook (generates proxy tags) → normalHooks → postHooks → injectNonceAttributeTagHook. The capture hook is registered as a plain transformIndexHtml function, which resolveHtmlTransforms places in normalHooks — i.e. after devHtmlHook — so it sees the generated proxy tags. Nonce handling is also correct: applyProxyScriptNonce runs before Vite's injectNonceAttributeTagHook, which only injects when no nonce attribute already exists, so there's no double nonce.
Issue 1 (significant): unbounded memory growth in the dev process
createPagesHtmlProxyCapturePlugin keeps two module-scoped maps:
const modules = new Map<string, CapturedProxyModule>();
const publicToResolvedId = new Map<string, string>();Entries are only ever added (pages-html-proxy.ts:176-178) — there is no eviction, size cap, watchChange/hotUpdate invalidation, or TTL. Because the key is a content hash of {code, document, index, map}, every render whose captured module source differs mints a new permanent entry. That is unbounded for exactly the scenarios this PR targets:
- alternating error/success renders of the same route (each captures a distinct module)
- stateful
order: prehooks (the added testcaptures stateful pre HTML hook output in immutable modulesproves each render yields a new URL/entry) - any request-dependent inline module (nonce, per-request data baked into the inline script)
Vite's own htmlProxyMap does not have this problem: it is keyed by filePath + index and overwrites in place, so it's bounded by the number of distinct document URLs. This PR replaces a bounded-by-URL cache with a grows-forever-by-content cache. A long-lived dev server hitting a page with a per-request nonce or changing data on every reload will leak steadily.
Suggested mitigations (any one):
- Bound retention per
(documentUrl, index)— e.g. keep only the last N captured hashes per document/index and drop older ones (matches the "immutable per render, but old renders are unreachable" reality; a stale tab could 404 on an evicted proxy, which is acceptable in dev and already possible after an HMR reload). - Evict a document's captured entries on
hotUpdate/watchChangefor files in that document graph. - At minimum, cap total map size (LRU) so the leak can't grow without limit.
Please also add a regression test asserting the cache is bounded (e.g. many distinct-content renders of one URL don't retain every entry).
Issue 2 (minor): misleading enforce: "pre"
The plugin sets enforce: "pre", but resolveHtmlTransforms ignores plugin-level enforce for transformIndexHtml entirely — it only branches on the hook's own order. The capture hook works because it's a plain function (→ normalHooks, after devHtmlHook), not because of enforce. enforce: "pre" only affects this plugin's resolveId/load ordering. That's probably fine (and may be intended for the content-module resolve/load), but the current setup makes the critical ordering invariant implicit. Consider a short comment documenting that the capture hook MUST stay a plain (normal-order) transformIndexHtml so it runs after devHtmlHook, since switching it to order: "pre" would silently break capture.
Issue 3 (nit): test fixture global state persists across the process
tests/fixtures/pages-basic/html-proxy-race-state.ts keeps a module-level races Map that never clears entries. Within the fixture server's lifetime that's harmless (each test uses a unique Date.now()-based race id), but it's another never-freed map living in the dev process for the whole suite. Not blocking; noting for symmetry with Issue 1.
Verification performed
vp test run tests/pages-html-proxy.test.ts— 10 passedvp check packages/vinext/src/server/pages-html-proxy.ts— format/lint/types clean- Traced Vite dev HTML transform ordering,
htmlProxyMapoverwrite semantics, andinjectNonceAttributeTagHookguard to confirm the fix's correctness and the nonce/ordering claims.
Overall: correct, well-tested fix for a real hydration-staleness bug. The memory-retention behavior (Issue 1) is the one thing I'd want addressed before merge, since it's a new unbounded leak introduced specifically on the dynamic-content paths this PR exercises.
|
Review posted to PR #2823. Summary of findings: Verdict: Correct fix for a real bug, with one issue I'd want addressed before mergeI traced the actual Vite ( Root cause confirmed — Vite's Issue 1 (significant): The plugin's Issue 2 (minor): Issue 3 (nit): Test fixture Verified locally: new test file (10 passed) and |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(dev): refresh Pages hydration after render errors
I re-traced the fix against the actual @voidzero-dev/vite-plus-core@0.2.6 dev HTML pipeline and module graph implementation (not the PR description), and checked that the two prior-review findings that were actionable have been resolved in commit 05ee82b.
Verdict: approve
The root-cause fix is correct and the follow-up commit closes the one blocking issue from the previous round. New tests pass (12) and vp check on the new source is clean.
Root cause + mechanism — confirmed sound
Vite's dev HTML proxy cache (EnvironmentModuleGraph, chunks/node.js:39642) keys inline proxies by filePath + index and addToHTMLProxyCache overwrites in place, so a slow error render and a fast success render to the same clean URL clobber each other's inline-proxy source while reusing the stable ?html-proxy&index=N.js URL. Capturing the exact loaded source, content-addressing it to an immutable __vinext_html_proxy_content_<hash>_<index>.js URL, and serializing same-clean-URL transforms with a per-document lock is the right fix.
- Hook ordering — the capture hook is a plain (normal-order)
transformIndexHtml, soresolveHtmlTransformsplaces it innormalHooks, afterdevHtmlHookgenerates the proxy tags. The added comment atpages-html-proxy.ts:237now documents that this MUST stay a normal-order hook (prior Issue 2). Good. - Nonce —
applyProxyScriptNoncestrips any existing nonce then re-adds the request nonce before Vite'sinjectNonceAttributeTagHook(which only injects when absent), so no double nonce. The pages-router test now asserts every immutable proxy tag carriesnonce="pages-response".
Prior Issue 1 (unbounded growth) — resolved
05ee82b replaces the grows-forever maps with a per-(documentUrl, index) LRU capped at MAX_RETAINED_PROXY_VERSIONS = 8 (retainProxyModule), plus evictViteModule which tears the evicted module out of the client module graph. I verified the internal map names against chunks/node.js:39644-39651 — urlToModuleMap, idToModuleMap, etagToModuleMap, fileToModulesMap, and _unresolvedUrlToModuleMap all match. _hasResolveFailedErrorModules is not cleared explicitly, but invalidateModule (called first) deletes the module from it (chunks/node.js:39713), so that reference is released too. The new bounds retained request-dependent proxy modules test drives 300 distinct renders and asserts ≤8 entries remain in both urlToModuleMap and _unresolvedUrlToModuleMap, and that the oldest URL 404s while the newest resolves.
The cap is per-index, so a 12-script document retains all 12 current indices (covered by retains every current proxy index in a document) while still bounding versions-per-index. Correct design.
Prior Issue 3 (fixture global) — resolved
html-proxy-race-state.ts now clears each race entry via queueMicrotask in releaseHtmlProxyRace (:71-73), so the fixture no longer accumulates state for the suite lifetime.
Notes (non-blocking)
- Eviction races with concurrent proxy fetches.
transformPagesHtmlserializes transforms per clean URL, but proxy-moduletransformRequest/loadcalls are not under that lock. A capture that evicts an old version can race a still-in-flight browser request for that exact evicted URL, yielding a dev-only 404. This is acceptable (same outcome as an HMR reload invalidating a stale tab) and matches the immutable-URL contract, but worth a one-line comment nearevictViteModulenoting the intended dev-only staleness window. _unresolvedUrlToModuleMaptransiently holds aPromise. For an in-flight resolution the map value is a Promise (chunks/node.js:39803), socandidate === modulewon't match it and that entry won't be swept. In practice the resolved node overwrites it (:39800) and the next eviction cleans it, so it doesn't leak beyond one cycle — fine, just flagging that the sweep relies on that overwrite behavior.
Parity
This is a Pages-Router-dev-only change. transformIndexHtml exists only in the Vite dev server; App Router dev goes through the RSC plugin, and prod/worker entries don't use the html-proxy mechanism. Both server.transformIndexHtml call sites in dev-server.ts were converted to transformPagesHtml; no other call sites exist. No parity gap.
Verification performed
vitest run tests/pages-html-proxy.test.ts— 12 passedvp check packages/vinext/src/server/pages-html-proxy.ts— format/lint/types clean- Confirmed Vite
EnvironmentModuleGraphinternal map names and_unresolvedUrlToModuleMapPromise-then-node overwrite semantics against the resolvedvite-plus-core@0.2.6source.
|
Review posted to PR #2823. Verdict: approveI re-traced the fix against the actual Root cause + mechanism — confirmed sound. Vite's dev html-proxy cache keys inline proxies by Prior Issue 1 (unbounded growth) — resolved. Replaced with a per- Prior Issues 2 & 3 — resolved via the documenting comment on the capture hook and Two non-blocking notes: the eviction can race a still-in-flight proxy fetch (dev-only 404, acceptable, worth a comment); and the Verified: |
|
/bigbonk review for issues |
1 participant
Summary
__NEXT_DATA__reuse the same immutable URLsRegression coverage
order: preHTML hook output captured after Vite processing__NEXT_DATA__values reuse proxy URLsNext.js recovery reference: https://github.com/vercel/next.js/blob/canary/test/development/acceptance/error-recovery.test.ts
Vite implementation reference: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/middlewares/indexHtml.ts
Validation
vp test run tests/pages-html-proxy.test.ts— 10 passedvp test run tests/pages-router.test.ts— 382 passedPLAYWRIGHT_PROJECT=pages-router-basepath-dev vp run test:e2e— 3 passedvp run vinext#buildpassed