[PR #392] perf: tee stream for ISR caching instead of full re-render #536

New issue

Open

opened 2026-05-06 13:08:38 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 13:08:38 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/cloudflare/vinext/pull/392
Author: @james-elicx
Created: 3/9/2026
Status: 🔄 Open

Base: main ← Head: fix/isr-stream-tee-no-double-render

📝 Commits (2)

438ac0d perf: tee stream for ISR caching instead of full re-render
d8a2275 test: update Pages Router server entry snapshot for stream tee ISR change

📊 Changes

3 files changed (+125 additions, -48 deletions)

View changed files

📝 packages/vinext/src/entries/pages-server-entry.ts (+28 -14)
📝 packages/vinext/src/server/dev-server.ts (+69 -20)
📝 tests/__snapshots__/entry-templates.test.ts.snap (+28 -14)

📄 Description

Problem

On every ISR cache miss, both the dev server and the production pages entry were doing a complete second renderToStringAsync of the page purely to get an HTML string for the cache. The HTML produced by the first streaming render was thrown away.

// Before (dev-server.ts:886–908)
await streamPageToResponse(res, element, ...);  // renders → streams → discards HTML

// Then re-renders the whole page just for the cache:
const isrBodyHtml = await renderToStringAsync(isrElement);
const isrHtml = `<!DOCTYPE html>...<div id="__next">${isrBodyHtml}</div>...`;
await isrSet(cacheKey, buildPagesCacheValue(isrHtml, pageProps), ...);

Same pattern existed in pages-server-entry.ts with an explicit comment acknowledging the trade-off.

Fix

Use ReadableStream.tee() to split the body stream at render time. One branch streams to the client; the other is collected for the cache. No second render.

`dev-server.ts`

streamPageToResponse() gains a collectHtml option. When set, it tees the bodyStream, pipes one branch to the Node.js response as before, drains the other into a string, and returns prefix + body + suffix to the caller. The ISR block now just calls isrSet with that returned string.

`pages-server-entry.ts` (production / Cloudflare Workers)

The compositeStream (already a full-document stream: shell prefix + body + shell suffix) is teed before the Response is returned. The cache branch is consumed in a background async IIFE — it does not block the streaming response to the client.

Result

ISR cache misses cost one render instead of two
TTFB is unchanged (the tee happens on the existing stream, not on a new one)
No behavior change for non-ISR routes (collectHtml defaults to false)

Testing

All 133 tests/pages-router.test.ts tests pass
All 23 tests/isr-cache.test.ts tests pass
Typecheck and lint clean

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/cloudflare/vinext/pull/392 **Author:** [@james-elicx](https://github.com/james-elicx) **Created:** 3/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/isr-stream-tee-no-double-render` --- ### 📝 Commits (2) - [`438ac0d`](https://github.com/cloudflare/vinext/commit/438ac0d45d3963ddf4ed4ce48155826ae97621a9) perf: tee stream for ISR caching instead of full re-render - [`d8a2275`](https://github.com/cloudflare/vinext/commit/d8a227510e747c8ea70127eb235dc86322a44751) test: update Pages Router server entry snapshot for stream tee ISR change ### 📊 Changes **3 files changed** (+125 additions, -48 deletions) <details> <summary>View changed files</summary> 📝 `packages/vinext/src/entries/pages-server-entry.ts` (+28 -14) 📝 `packages/vinext/src/server/dev-server.ts` (+69 -20) 📝 `tests/__snapshots__/entry-templates.test.ts.snap` (+28 -14) </details> ### 📄 Description ## Problem On every ISR cache miss, both the dev server and the production pages entry were doing a **complete second `renderToStringAsync`** of the page purely to get an HTML string for the cache. The HTML produced by the first streaming render was thrown away. ``` // Before (dev-server.ts:886–908) await streamPageToResponse(res, element, ...); // renders → streams → discards HTML // Then re-renders the whole page just for the cache: const isrBodyHtml = await renderToStringAsync(isrElement); const isrHtml = `<!DOCTYPE html>...<div id="__next">${isrBodyHtml}</div>...`; await isrSet(cacheKey, buildPagesCacheValue(isrHtml, pageProps), ...); ``` Same pattern existed in `pages-server-entry.ts` with an explicit comment acknowledging the trade-off. ## Fix Use `ReadableStream.tee()` to split the body stream at render time. One branch streams to the client; the other is collected for the cache. No second render. ### `dev-server.ts` `streamPageToResponse()` gains a `collectHtml` option. When set, it tees the `bodyStream`, pipes one branch to the Node.js response as before, drains the other into a string, and returns `prefix + body + suffix` to the caller. The ISR block now just calls `isrSet` with that returned string. ### `pages-server-entry.ts` (production / Cloudflare Workers) The `compositeStream` (already a full-document stream: shell prefix + body + shell suffix) is teed before the `Response` is returned. The cache branch is consumed in a background async IIFE — it does not block the streaming response to the client. ## Result - ISR cache misses cost **one render** instead of two - TTFB is unchanged (the tee happens on the existing stream, not on a new one) - No behavior change for non-ISR routes (`collectHtml` defaults to `false`) ## Testing - All 133 `tests/pages-router.test.ts` tests pass - All 23 `tests/isr-cache.test.ts` tests pass - Typecheck and lint clean --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>