fix(pages): preserve styled-jsx CSS ordering in SSR - #2894
Conversation
|
/bigbonk review for issues |
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 |
Review:
|
|
Review posted to PR #2894. Summary of my findings: Review SummaryThe PR is well-implemented and thoroughly tested. No blocking issues found. I verified locally that all relevant tests pass (13 styled-jsx, 137 pages-response/data, 5 pages-router streaming/ISR) and Verified correct
Non-blocking observations (optional clarifying comments)
|
|
/bigbonk review for issues |
Review:
|
|
Review posted to PR #2894. Summary of findings: Review SummaryThorough review of the full diff, dev/prod parity across all four request-handling paths, and verification of the styled-jsx runtime shape against pinned Key findings
Non-blocking observations (optional clarifying comments)
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve styled-jsx during streaming SSR
Independent pass over the full diff, with source-level verification against the pinned styled-jsx@5.1.6 runtime and dev/prod parity across renderPagesPageResponse, streamPageToResponse, _renderIsrPassToStringAsync, and the ISR regeneration path. Ran the relevant suites locally — all green (14 styled-jsx, 137 pages-data/response) — and vp check is clean on all six changed source files. No blocking issues found.
Verified against the actual runtime
JSXStyleregisters styles synchronously during SSR render (styled-jsx/dist/index/index.js:472-474:if (typeof window === "undefined") { registry.add(props); return null; }). This is the load-bearing fact for the whole design: shell styles are all present whenrenderToReadableStreamresolves the shell, so the eagercollectPagesStyleRegistryHtml→<head>collection is complete, and Suspense/late styles register only as their boundaries render, so gating the second collection onbodyAllReadycaptures exactly those. No overlap.flush()clears_indices/_instancesCounts(:373-379), so the eager collect-then-flush cannot double-emit into the late collection. Confirmed.registry.styles()always returns an array (mapRulesToStyleuses.map(),:288-303), so both the prod entry'sstyles.length > 0guard andcollectPagesStyleRegistryHtml'sArray.isArray(styles) && styles.length === 0check are safe.STYLED_JSX_STYLE_PATTERNid prefix is correct:computeIdreturns"jsx-" + baseId(:260) andmapRulesToStyleprepends"__"(:294), yieldingid="__jsx-…", which the lookaheadid=["']__jsx-matches.convertStyledJsxRuntimeToEsmmatches the pinned runtime. The file tail is exactlyexports.StyleRegistry = …; exports.createStyleRegistry = …; exports.style = JSXStyle; exports.useStyleRegistry = useStyleRegistry;\n, so all three replacements hit; the fail-loudthrowon any no-op is a sound guard against future layout drift.
Verified correct
- ISR
tee()has no backpressure deadlock.bodyAllReadyis captured beforetee();finalShellSuffixawaitslateStyledJsxHTML → bodyAllReady. Both consumers drive the render independently —writePagesIsrCachedrains the cache branch viareadStreamAsTextandbuildPagesCompositeStreamdrains the response branch — and both await the samefinalShellSuffix. Neither waits on an undrained branch. The bot path bufferscompositeStream(which already awaitsfinalShellSuffix), so late styles land in the buffered/ETag'd HTML too. disableNextSsg: truefixes a real bug. Without it SWC's client-page tree-shaker rewritesgetStaticProps/getServerSidePropsto__N_SSG/__N_SSPand strips the functions from vinext's server module graph. The newstyled-jsx-static-props/styled-jsx-streamingfixtures and thenot.toContain("__N_SSP"/"__N_SSG")transform assertions lock this in.renderErrorPagerefactor is behavior-preserving. The removed non-Documentbranch is subsumed bystreamPageToResponse'selsedefault-shell path (dev-server.ts:513-529), andpreserveExistingContentType: statusCode === 404reproduces the old conditional Content-Type. Error pages additionally gain styled-jsx.- ISR head refresh is complete.
removeStyledJsxStylesis global/unanchored so it strips__jsx-styles from both<head>and the pre-</body>region on every regen (including when the fresh render emits none), then re-inserts the fresh run into<head>. Thedocument-isr-styledandisr-second-render-state(z-index: ${timestamp}) fixtures prove the styles actually advance and the stale ones are gone (not.toMatch(z-index: ${firstTimestamp})). - Fallback runtime is a coherent no-op. No
next→STYLED_JSX_FALLBACK_RUNTIME(empty registry,styles()→[]) andstyled-jsx/style→ null; the generated entry's static import never crashes and emits no styles.
Non-blocking observations (agree with prior review)
- Dev/prod encode different optionality for the styled-jsx import. Prod (
pages-server-entry.ts) statically importsstyled-jsx; dev (dev-server.ts:365) wraps it in try/catch. Both are safe because the plugin always resolves the id (real runtime or fallback), but a one-line comment on the prod static import stating why it is unconditionally safe would stop a future reader from "fixing" the asymmetry into an optional import (which would silently drop styles). STYLED_JSX_STYLE_PATTERNandconvertStyledJsxRuntimeToEsmare both coupled to the pinned runtime's shape (id-first attribute ordering / export tail). The ESM converter fails loud on drift; the style pattern does not. Acceptable given styled-jsx is pinned vianext.
Nothing here blocks merge.
|
Review posted to PR #2894. SummaryI performed an independent review of PR #2894 ( Key verifications (against runtime source)
Verified correct
Non-blocking (optional)
Tests pass locally (14 styled-jsx, 137 pages-data/response) and |
|
Independent exact-head review of I reviewed the full 13-file diff independently, including:
Fresh validation on this immutable head:
I found no actionable streaming/lifecycle gap, virtual-runtime or NUL-filter regression, dev/production/static-export divergence, or missing high-value test at this head. |
|
/bigbonk review for issues |
Review:
|
|
Review posted to PR #2894. SummaryI did an independent review of PR #2894 ( Verified correct
Non-blocking findings I raised (not covered by prior reviews)
Items 1 and 2 are the most valuable to address so the code and its tests stay honest about the streaming marker's current (non-)role. |
63e79a8 to
c423475
Compare
|
/bigbonk review for issues |
Review:
|
|
Review posted to PR #2894. SummaryI did an independent review of PR #2894 ( Key verifications
Non-blocking observations
All items are optional clarifying comments; nothing blocks merge. |
Summary
allReadyin dev and production, matching Next.js and preventing later global CSS resources from overriding styled-jsx rulesContent-Lengthvalues before Worker response mergingPages Router behavior change
Pages Router Suspense HTML is intentionally no longer exposed progressively: body bytes wait for
renderStream.allReady. This trades Pages Router TTFB and progressive reveal for deterministic CSS ordering and Next.js parity. App Router streaming is unchanged, and PagesHEADrequests retain their headers-only fast path.Validation
vp checkon all changed source and test filestest/e2e/streaming-ssr/index.test.ts: 5/5 passedtest/e2e/app-dir/scss/with-styled-jsx/with-styled-jsx.test.ts: 2/2 passed across both Sass variantsReview
Two independent cumulative reviews returned no findings after the raw-text, Worker header, duplicate styled-jsx registry, behavior-contract, and dead HTML streaming-marker findings were addressed. The PR description was also replaced and verified from a literal body file so the documented commands and behavior remain intact.