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 |
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