Skip to content

Commit 0bcdbe4

Browse files
mhartgnoff
andauthored
Ensure getServerInsertedHTML skips rendering correctly (#85394)
### Problem Both `polyfillTags` and `traceMetaTags` are only rendered if they haven't already been flushed, however the current check to skip rendering (lines 81-89) doesn't take this into account – only the render logic does (lines 93-98). This results in `renderToReadableStream` being called (potentially thousands of times – [for each chunk](https://github.com/vercel/next.js/blob/498349c375e2602f526f64e8366992066cfa872c/packages/next/src/server/stream-utils/node-web-streams-helper.ts#L368) in the [head insertion transform stream in the transformer chain](https://github.com/vercel/next.js/blob/498349c375e2602f526f64e8366992066cfa872c/packages/next/src/server/stream-utils/node-web-streams-helper.ts#L834)), constructing new `ReadableStream`s each time, just to render empty strings. It's present in canary/v16 and I suspect it was introduced in v15 (so can be backported). The logic check for v14 looks correct. ### Solution This PR ensures the shortcut check lines up with the render logic by just zeroing tag arrays after they've been used. There are a number of ways to achieve the same result – including just adding the `hasFlushedInitially` check to the shortcut logic – however this would result in the logic still being split in two places, so I've chosen one way to ensure the logic is centralized. Happy with whatever method is preferred here. ### Results This results in a 10-38% perf improvement end-to-end, especially for large renders. Eg, in [theo's benchmark](https://github.com/t3dotgg/cf-vs-vercel-bench/tree/main/next-bench) it brings the number of `renderToReadableStream` calls from 1745 here down to just 1, which has a noticeable impact on end-to-end performance I've [created a repo to highlight/reproduce the problem](https://github.com/mhart/next-server-inserted-fix) based off that benchmark. The results from there: - Node.js: 1.18x faster - Deno: 1.38x faster - workerd: 1.24x faster - Bun: 1.15x faster --------- Co-authored-by: Josh Story <story@hey.com>
1 parent 69e1989 commit 0bcdbe4

File tree

2 files changed

+16
-21
lines changed

2 files changed

+16
-21
lines changed

packages/next/src/server/app-render/make-get-server-inserted-html.tsx

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,14 @@ export function makeGetServerInsertedHTML({
2626
basePath: string
2727
}) {
2828
let flushedErrorMetaTagsUntilIndex = 0
29-
// flag for static content that only needs to be flushed once
30-
let hasFlushedInitially = false
3129

32-
const polyfillTags = polyfills.map((polyfill) => {
30+
// These only need to be rendered once, they'll be set to empty arrays once flushed.
31+
let polyfillTags = polyfills.map((polyfill) => {
3332
return <script key={polyfill.src} {...polyfill} />
3433
})
34+
let traceMetaTags = (tracingMetadata || []).map(({ key, value }, index) => (
35+
<meta key={`next-trace-data-${index}`} name={key} content={value} />
36+
))
3537

3638
return async function getServerInsertedHTML() {
3739
// Loop through all the errors that have been captured but not yet
@@ -69,12 +71,6 @@ export function makeGetServerInsertedHTML({
6971
}
7072
}
7173

72-
const traceMetaTags = (tracingMetadata || []).map(
73-
({ key, value }, index) => (
74-
<meta key={`next-trace-data-${index}`} name={key} content={value} />
75-
)
76-
)
77-
7874
const serverInsertedHTML = renderServerInsertedHTML()
7975

8076
// Skip React rendering if we know the content is empty.
@@ -90,12 +86,9 @@ export function makeGetServerInsertedHTML({
9086

9187
const stream = await renderToReadableStream(
9288
<>
93-
{
94-
/* Insert the polyfills if they haven't been flushed yet. */
95-
hasFlushedInitially ? null : polyfillTags
96-
}
89+
{polyfillTags}
9790
{serverInsertedHTML}
98-
{hasFlushedInitially ? null : traceMetaTags}
91+
{traceMetaTags}
9992
{errorMetaTags}
10093
</>,
10194
{
@@ -105,7 +98,9 @@ export function makeGetServerInsertedHTML({
10598
}
10699
)
107100

108-
hasFlushedInitially = true
101+
// The polyfills and trace metadata have been flushed, so they don't need to be rendered again
102+
polyfillTags = []
103+
traceMetaTags = []
109104

110105
// There's no need to wait for the stream to be ready
111106
// e.g. calling `await stream.allReady` because `streamToString` will

test/e2e/app-dir/cache-components-errors/cache-components-errors.test.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3251,13 +3251,13 @@ describe('Cache Components Errors', () => {
32513251
at ScrollAndFocusHandler (bundler:///<next-src>)
32523252
at RenderFromTemplateContext (bundler:///<next-src>)
32533253
at OuterLayoutRouter (bundler:///<next-src>)
3254-
339 | */
3255-
340 | function InnerLayoutRouter({
3256-
> 341 | tree,
3254+
332 | */
3255+
333 | function InnerLayoutRouter({
3256+
> 334 | tree,
32573257
| ^
3258-
342 | segmentPath,
3259-
343 | debugNameContext,
3260-
344 | cacheNode,
3258+
335 | segmentPath,
3259+
336 | debugNameContext,
3260+
337 | cacheNode,
32613261
To get a more detailed stack trace and pinpoint the issue, start the app in development mode by running \`next dev\`, then open "/use-cache-private-without-suspense" in your browser to investigate the error.
32623262
Error occurred prerendering page "/use-cache-private-without-suspense". Read more: https://nextjs.org/docs/messages/prerender-error
32633263

0 commit comments

Comments
 (0)