fix: Pages Router prod hydration for inlined page modules#394
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Pages Router prod hydration fix for inlined page modules
The diagnosis is correct and the fix is well-targeted. When Vite inlines a page module into the client entry chunk, its ssr-manifest.json entry becomes empty — the server then renders HTML but never injects the client JS, causing a silent hydration miss. The augmentSsrManifestFromBundle approach of walking the emitted bundle to rebuild these mappings is sound.
The test coverage is thorough: unit tests for the augmentation function, integration tests for basePath prefixing, CSS/asset backfilling, and end-to-end production server assertions.
Main concern is the triplication of normalizeManifestFile / manifestFileWithBase across three files. A few smaller items below.
| key, | ||
| new Set(files.map((file) => normalizeManifestFile(file))), | ||
| ]), | ||
| ) as Record<string, Set<string>>; |
There was a problem hiding this comment.
Nit: The as Record<string, Set<string>> cast is unnecessary since Object.fromEntries + the .map already produce the correct type. TypeScript infers Record<string, Set<string>> from the map callback. Not a blocker.
|
|
||
| for (const moduleId of Object.keys(chunk.modules ?? {})) { | ||
| const key = normalizeManifestModuleId(moduleId, root); | ||
| if (key.startsWith("node_modules/") || key.includes("/node_modules/")) continue; |
There was a problem hiding this comment.
Good filtering — skipping node_modules/ entries prevents bloating the manifest. Worth noting: module IDs starting with \0 (Rollup virtual module prefix, like \0virtual:vinext-client-entry) will pass this filter and get added to the manifest. They're harmless since they'll never match a page lookup key, but you could skip them explicitly for cleanliness:
| if (key.startsWith("node_modules/") || key.includes("/node_modules/")) continue; | |
| if (key.startsWith("node_modules/") || key.includes("/node_modules/")) continue; | |
| if (key.startsWith("\0")) continue; |
|
|
||
| for (const item of Object.values(bundle)) { | ||
| if (item.type !== "chunk") continue; | ||
| const chunk = item as BundleBackfillChunk; |
There was a problem hiding this comment.
The as BundleBackfillChunk cast after the item.type !== "chunk" guard is safe, but could be cleaner with a type predicate or narrowing the union type. Minor nit, not blocking.
|
|
||
| // Extract config values (embedded at build time in the server entry) | ||
| const basePath: string = vinextConfig?.basePath ?? ""; | ||
| const assetBase = basePath ? `${basePath}/` : "/"; |
There was a problem hiding this comment.
Good change: computing assetBase from basePath and using it to prefix lazy chunks ensures parity with the Cloudflare closeBundle path. One subtlety — the closeBundle hook in index.ts reads envConfig.base (Vite's base config), while here you derive it from vinextConfig.basePath (Next.js config). These should always match since vinext sets Vite's base from basePath, but it's worth keeping in mind if they ever diverge.
| ([key]) => key.endsWith("/pages/counter.tsx") || key === "pages/counter.tsx", | ||
| ); | ||
| expect(counterManifestEntry).toBeDefined(); | ||
| expect(counterManifestEntry?.[1].some((file: string) => file.endsWith(".js"))).toBe(true); |
There was a problem hiding this comment.
Good assertion — verifying the counter page has JS files in its manifest entry proves the backfill worked for the inlined case. One thing missing: there's no assertion that the entry was actually empty before the backfill (i.e., that Vite really did inline the counter into the entry chunk). The test would be more robust if it also checked the build manifest (manifest.json) to confirm counter.tsx has no standalone entry, proving the backfill was necessary. That said, the counterBuildManifestEntries check in the basePath test at line 1223 does exactly this — consider adding the same guard here.
|
Review submitted. Here's a summary of the feedback: Overall: The fix is correct and well-targeted. The Key feedback points:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: Pages Router prod hydration fix for inlined page modules
This is a well-diagnosed fix for a real production bug. The approach — backfilling ssr-manifest.json from the emitted client bundle in a writeBundle hook — is correct and minimally invasive. The utility extraction to utils/manifest-paths.ts addresses the earlier review's main concern about code triplication.
What I verified:
- The
writeBundlehook correctly runs only for the client build (guarded byssr-manifest.jsonexistence, which is only emitted withssrManifest: trueon the client build config) - The hook runs before
closeBundle, so the Cloudflare build integration picks up the augmented manifest assetBasederivation inprod-server.ts(${basePath}/) matches what vinext sets as Vite'sbase(nextConfig.basePath + "/") — these stay in sync- The
\0virtual module prefix skip (line 644) is exercised by the first unit test ("\0virtual:vinext-client-entry"in the modules map) - All three former copies of
normalizeManifestFile/manifestFileWithBasenow import from the sharedutils/manifest-paths.ts normalizeManifestModuleIdcorrectly handles both POSIX and Windows absolute paths, with a fallback to the normalized ID for paths outside the root
Test coverage is thorough across all layers: unit tests for augmentSsrManifestFromBundle, integration tests for basePath prefixing and CSS/asset backfilling, and end-to-end production server assertions.
Two minor comments below, neither blocking.
| key, | ||
| new Set(files.map((file) => normalizeManifestFile(file))), | ||
| ]), | ||
| ) as Record<string, Set<string>>; |
There was a problem hiding this comment.
Nit: the as Record<string, Set<string>> cast isn't needed here — TypeScript infers the correct type from Object.fromEntries + the .map callback. Same applies to line 653 with as Record<string, string[]>. Not blocking, but removing these would be slightly cleaner.
|
|
||
| for (const item of Object.values(bundle)) { | ||
| if (item.type !== "chunk") continue; | ||
| const chunk = item as BundleBackfillChunk; |
There was a problem hiding this comment.
Minor: after the item.type !== "chunk" guard, Rollup's OutputChunk type should narrow automatically if you annotate the bundle parameter with the Rollup output types. The as BundleBackfillChunk cast is safe but could be avoided by narrowing the union in the bundle parameter type. Not blocking — the custom BundleBackfillChunk type is pragmatic here since it only declares the fields you actually use.
|
Review submitted and approved. Here's a summary of my findings: Verdict: Approved The fix is correct and well-targeted. When Vite inlines a page module into the client entry chunk, its Key things I verified:
Two non-blocking nits on unnecessary type casts in |
2 participants
This fixes a Pages Router bug where SSR worked but interactive pages failed to hydrate when the page module was inlined into the client entry chunk.
For affected routes like
/counter, the server looked up the page inssr-manifest.jsonto decide which client assets to inject. When Vite inlined that page into the main client entry, the page’s SSR manifest entry was emitted as empty, so the server rendered HTML but never loaded the page’s client code. The result was a silent hydration miss in production on Node.The fix backfills
ssr-manifest.jsonfrom the emitted client bundle rather than relying only onmanifest.json. That lets us map source page modules to the chunk that actually contains them, including:basePath/ Vitebase-prefixed asset URLsTest coverage was expanded to prove the runtime path:
/countercasebasePath-prefixed emitted asset tagsVerification:
HEAD:/counterstayed atCount: 0andssr-manifest.json["pages/counter.tsx"]was empty/counterupdates toCount: 1