fix(app-router): ensure streamed SSR body ends with </body></html> (#1532) - #1624
Conversation
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): ensure streamed SSR body ends with </body></html>
Clean, well-scoped fix. The approach correctly mirrors Next.js's createMoveSuffixStream — strip the first </body></html> from the stream during chunk processing and unconditionally re-emit it in flush() after all trailing flight data and preinit scripts. I verified the Next.js source and the semantics match exactly, including the defense-in-depth behavior of appending the suffix even when the source stream omits it.
What I checked
- Next.js parity: Confirmed against
createMoveSuffixStreaminpackages/next/src/server/stream-utils/node-web-streams-helper.ts. Same strip-then-reappend pattern, same unconditional emit inflush(), same comment rationale for the missing-suffix case. - Existing test compatibility: The
runTransformhelper tests (e.g., line 270expect(out).toBe(html)) still pass because the suffix is stripped and re-appended, producing identical output for well-formed input. - Mid-stream RSC scripts: RSC scripts emitted by
rscEmbed.flush()in thetransform()timeout (line 306) land afterflushBufferedhas already stripped the suffix from that batch, so they can't accidentally appear after</body></html>. Correct ordering. flush()ordering:flushBuffered→ fallbackemitInsertion→rscEmbed.finalize()→ suffix. All trailing content lands before the closing tags.
One edge case worth noting (not blocking)
The suffix detection operates on individual decoded string chunks (chunk.indexOf(DOCUMENT_CLOSE_SUFFIX)). If </body></html> were ever split across two Fizz chunks (e.g., </bo in one chunk and dy></html> in the next), the indexOf would miss it. Next.js has the same theoretical limitation with indexOfUint8Array on individual chunks. In practice, React Fizz emits the closing shell as a single chunk and the tick-buffering in this transform coalesces multiple chunks before scanning, so this is not a real concern — just documenting that the trade-off is understood and matches upstream.
Tests are thorough: unit tests cover the happy path (trailing scripts reordered), the no-<head> fallback path, and the missing-suffix defense-in-depth case. The integration test exercises a real App Router render and asserts both endsWith and single-occurrence. Good coverage.
|
Review posted and approved on PR #1624. Here's the summary: Approved — clean, well-scoped fix that correctly mirrors Next.js's The fix strips the first Key things verified:
|
|
Updated |
|
Pushed biome formatting fix for |
…1532) Trailing flight chunks and preinit scripts emitted by `rscEmbed.finalize()` in the tick-buffered transform's `flush()` were being appended after React Fizz's `</body></html>` closing tags, leaving the streamed document body ending in `<script>...</script>` rather than a well-formed close. Strip the first `</body></html>` we see in the buffered HTML chunks and re-emit it at the very end of `flush()`, after the final RSC scripts. Mirrors Next.js's `createMoveSuffixStream` behaviour (packages/next/src/server/stream-utils/node-web-streams-helper.ts). Ported from Next.js test: test/e2e/app-dir/app/index.test.ts — "should ensure the </body></html> suffix is at the end of the stream". Fixes #1532.
The 'delivers RSC chunks progressively' case still asserted the old order where the navigation-runtime done signal trailed </html>. After #1532 suffix-moved </body></html> to the very end of the stream, the done signal sits *between* the body content and the document close. Update the assertion to match: done > last body content, and </html> > done.
a00dcd4 to
da7fec9
Compare
|
Rebased on origin/main; resolved conflict in |
1 participant
Summary
The App Router streaming SSR pipeline emitted trailing flight chunks and preinit scripts (from
rscEmbed.finalize()) after React Fizz's</body></html>closing tags, leaving the body ending in<script>...</script>instead of a well-formed document close. This brokebody.endsWith('</body></html>')consumer assertions.The fix mirrors Next.js's
createMoveSuffixStream: strip the first</body></html>we see in buffered chunks and re-emit it at the very end of the transform'sflush(), after the final RSC scripts.packages/vinext/src/server/app-ssr-stream.ts(~25 lines of net logic + comments).tests/app-ssr-stream.test.tscover trailing-script reorder, the no-<head>fallback path, and the missing-suffix defense-in-depth.tests/app-router.test.tsexercises a real App Router render and assertshtml.endsWith('</body></html>')plus that the suffix appears only once.Reference (Next.js):
createMoveSuffixStreamandCLOSE_TAGFixes #1532.
Test plan
pnpm test tests/app-ssr-stream.test.ts— 21 tests pass (3 new)pnpm test tests/app-router.test.ts tests/app-page-stream.test.ts— 347 tests pass (1 new)pnpm test tests/features.test.ts— 309 tests passpnpm run check— formatting, lint, types clean