fix(dev): refresh Pages hydration after render errors - #2823
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 |
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 |
Review: fix(dev): refresh Pages hydration after render errors (re-review at
|
|
Review posted to PR #2823. SummaryI re-reviewed the fix, focusing on the newest commit Root cause fix — still correct. Capturing Vite's exact inline proxy source, content-addressing to an immutable URL, and per-document lock serialization is the right approach. The One new finding (moderate, not blocking): The Things that check out:
Verdict: correct fix; the newest commit resolves the last non-blocking note. Recommend addressing the cross-document retention growth (edit-driven eviction + a document-count bound) before merge, but not strictly blocking. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(dev): refresh Pages hydration after render errors (re-review at 3814400)
I re-traced the fix and focused on the newest commit 3814400 ("bound retained HTML proxy documents") on top of the previously-reviewed bc4c141. This commit is a direct, correct response to the moderate finding I raised in the last round (retention bounded per document but unbounded across documents).
Verdict: approve
The root-cause fix remains correct, and the cross-document retention leak is now closed. vp check on the new source is clean and all tests pass (15 in pages-html-proxy.test.ts, 382 in pages-router.test.ts).
Prior finding (cross-document unbounded growth) — resolved
3814400 replaces retainedByDocumentIndex: Map<string, Set> with a nested retainedByDocument: Map<documentUrl, Map<index, Set<resolvedId>>> and adds a second LRU dimension:
- Per-
(documentUrl, index)version cap stays atMAX_RETAINED_PROXY_VERSIONS = 8(unchanged). - New per-document cap
MAX_RETAINED_PROXY_DOCUMENTS = 128, with LRU-refresh on touch (retainedByDocument.delete(documentUrl)+ re-setatpages-html-proxy.ts:246-252). When the document count exceeds 128, the oldest document's every retained version is evicted through the sharedevictRetainedProxyModulehelper (:270-280). hotUpdatehandler (:351-356) evicts all retained proxies and clearsretainedByDocumenton any client-graph file change — over-invalidates, but that's safe in dev and matches "immutable per render, superseded on edit."
The eviction path was also refactored into evictRetainedProxyModule, which now drops the modules entry, the publicToResolvedId aliases, and the Vite module graph node in one place — so both LRU dimensions and hotUpdate share identical teardown. Good.
New regression tests cover both axes:
bounds retained modules across distinct document URLsdrives 160 distinct/docs/NURLs and asserts ≤128 retained graph modules, oldest 404s, newest resolves.evicts retained proxies when the source graph updatescalls thehotUpdatehandler and asserts the captured module is torn down.
Things I re-verified
hotUpdatesignature —HotUpdateOptionsincludesserver: ViteDevServer(vite-plus-core@0.2.6index.d.ts:1198-1205), sohotUpdate({ server })is valid. The handler is idempotent (every eviction null-checks), so per-environment / multi-fire invocation is safe.- Pending-resolution eviction (
bc4c141) —evictViteModulesweeps_unresolvedUrlToModuleMap, and the.then(onFulfilled, onRejected)has both handlers withvoid, so no unhandled-rejection risk; the rejection branch only deletes if the entry hasn't been overwritten. - Concurrency — different document URLs transform concurrently and share
modules/retainedByDocument, butretainProxyModulehas noawaitinternally, so no torn state. Document-LRU eviction only removes other documents; worst case is an evict-then-re-add, which is harmless. - Cross-document isolation — the content hash
identitystill includesdocument: active.documentUrl(:315), so byte-identical inline content in sibling routes gets distinct resolvedIds; no cross-document eviction hazard. - Dev/prod parity — both Pages
server.transformIndexHtmlcall sites indev-server.ts(:600,:2155) are converted totransformPagesHtml;renderErrorPagenow forwardsscriptNoncetostreamPageToResponse(:2098) closing a nonce gap on error-page hydration.transformIndexHtmlis dev-only, so no worker/prod parity gap. - Prior Issues 2 & 3 — normal-order hook comment and fixture
queueMicrotaskcleanup remain in place.
Non-blocking notes
hotUpdateis a blunt invalidation. It clears all documents' captured proxies on any file change, not just files in the changed document's graph. In dev that's fine (worst case an in-flight stale tab 404s), and it's strictly safe, but afilteror file-scoped eviction would be more surgical if this ever shows up as churn on large apps. Not worth blocking.MAX_RETAINED_PROXY_DOCUMENTS = 128×8versions × N indices is the new theoretical ceiling. That's bounded and reasonable for a dev process; just noting the constants are the tuning surface if memory ever matters.
Verification performed
vitest run tests/pages-html-proxy.test.ts— 15 passed.vitest run tests/pages-router.test.ts— 382 passed.vp check packages/vinext/src/server/pages-html-proxy.ts— format/lint/types clean.- Confirmed
HotUpdateOptions.serverand the nested-map LRU eviction paths against the resolvedvite-plus-core@0.2.6source.
Correct fix for the hydration-staleness bug; the newest commit closes the last outstanding retention finding on both the version and document axes.
|
Review posted to PR #2823. Verdict: approveI re-reviewed the fix focusing on the newest commit Cross-document leak — resolved.
Two new regression tests cover both axes (160 distinct doc URLs → ≤128 retained; Re-verified: Non-blocking notes: Verification: Correct fix; the newest commit closes the last outstanding retention finding on both the version and document axes. |
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