fix: serialize SSR render and head collection in Pages Router (#792)#951
Conversation
…lare#792) Move renderToReadableStream() before buildPagesShellHtml() + clearSsrContext() so that <Head> and other SSR state collectors (styled-jsx, useServerInsertedHTML) are populated before they are read. This fixes a race condition where head styles were silently dropped because they were collected before the page had finished rendering. Mirrors Next.js fix: vercel/next.js@9853944 The specific styled-jsx race condition (styledJsxInsertedHTML() called concurrently with page render via Promise.all) does not exist in vinext because vinext does not implement a styled-jsx style registry at all. However, the same class of ordering bug affected vinext's Pages Router prod path where getSSRHeadHTML was called before rendering completed.
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.
The fix is correct and important. Moving renderToReadableStream before buildPagesShellHtml ensures <Head> content is populated before getSSRHeadHTML() collects it. This brings pages-page-response.ts in line with dev-server.ts, which already had the correct ordering (render at line 117, collect head at line 120, clear context at line 978).
A couple of minor nits below, neither blocking.
| // (e.g. styled-jsx, useServerInsertedHTML) are populated before we read | ||
| // them. This fixes a race condition where head styles were silently dropped | ||
| // because they were collected before the page had finished rendering. | ||
| // Mirrors Next.js fix: vercel/next.js@9853944 |
There was a problem hiding this comment.
Nit: the comment says "Mirrors Next.js fix: vercel/next.js@9853944" — this is a bare commit hash with no hyperlink, which makes it hard to look up. Consider making it a full URL:
| // Mirrors Next.js fix: vercel/next.js@9853944 | |
| // Mirrors Next.js fix: https://github.com/vercel/next.js/commit/9853944 |
| expect(callOrder).toEqual(["render", "head"]); | ||
| }); | ||
|
|
||
| it("clears SSR context only after rendering, not before", async () => { |
There was a problem hiding this comment.
Nit: the test name says "clears SSR context only after rendering, not before" — but in the old code, clearSsrContext was already called after buildPagesShellHtml (which itself was after getSSRHeadHTML). The actual bug was that renderToReadableStream came after both of those, not that clearSsrContext was called too early. The test is still valid as a regression guard for the new ordering, but the name slightly mischaracterizes the original bug. Something like "renders page before clearing SSR context" would be more precise.
There was a problem hiding this comment.
The fix is correct and important. Moving renderToReadableStream before buildPagesShellHtml ensures <Head> content is populated before getSSRHeadHTML() collects it. This brings pages-page-response.ts in line with dev-server.ts, which already had the correct ordering (render at line 117, collect head at line 120, clear context at line 978).
A couple of minor nits below, neither blocking.
| // (e.g. styled-jsx, useServerInsertedHTML) are populated before we read | ||
| // them. This fixes a race condition where head styles were silently dropped | ||
| // because they were collected before the page had finished rendering. | ||
| // Mirrors Next.js fix: vercel/next.js@9853944 |
There was a problem hiding this comment.
Nit: the comment says "Mirrors Next.js fix: vercel/next.js@9853944" — this is a bare commit hash with no hyperlink, which makes it hard to look up. Consider making it a full URL:
| // Mirrors Next.js fix: vercel/next.js@9853944 | |
| // Mirrors Next.js fix: https://github.com/vercel/next.js/commit/9853944 |
| expect(callOrder).toEqual(["render", "head"]); | ||
| }); | ||
|
|
||
| it("clears SSR context only after rendering, not before", async () => { |
There was a problem hiding this comment.
Nit: the test name says "clears SSR context only after rendering, not before" — but in the old code, clearSsrContext was already called after buildPagesShellHtml (which itself was after getSSRHeadHTML). The actual bug was that renderToReadableStream came after both of those, not that clearSsrContext was called too early. The test is still valid as a regression guard for the new ordering, but the name slightly mischaracterizes the original bug. Something like "renders page before clearing SSR context" would be more precise.
|
Review submitted. Here's a summary: Approved the PR with two minor nits:
|
2 participants
Fixes #792
Summary
styledJsxInsertedHTML()(no styled-jsx style registry exists), so the specific Next.js bug does not applygetSSRHeadHTML()was called beforerenderToReadableStream(), causing<Head>content to be silently dropped from SSR HTMLclearSsrContext()ordering: SSR context was cleared before rendering, souseRouter()would return defaults during SSRTest plan
pages-page-response.test.tsthat verifyrenderToReadableStreamis called beforegetSSRHeadHTMLandclearSsrContextpages-page-responsetests (7 total) passheadtests (30) passpages-router.test.ts(196 tests) pass