fix: replay render-time response headers for cached App Router responses#488
Conversation
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7242c926f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Render-time response header cache parity
This is a well-structured change that brings cached App Router responses significantly closer to Next.js behavior by preserving and replaying render-time response headers losslessly. The architecture is sound: unified multi-value header collector → lossless serialization → deterministic merge on replay. The test coverage is thorough, spanning MISS/HIT/STALE/regen-HIT for both HTML and RSC-first paths, plus edge cases like middleware merge, Vary dedup, WWW-Authenticate replay, late async render headers, and special responses (redirect/notFound).
Below are the issues I found, roughly ordered by severity.
P1: RSC-first MISS awaits full stream capture before returning — defeats streaming
The Codex bot flagged this and it's a real concern. On the RSC-first MISS path (line ~2673), the code now does:
const __rscDataForResponse = await __isrRscDataPromise;
const __responseRenderHeaders = consumeRenderResponseHeaders();…then constructs the response from the fully-captured __rscDataForResponse buffer.
Previously, this path returned new Response(__rscForResponse, ...) where __rscForResponse was the streaming half of the tee'd ReadableStream. The client could start consuming chunks immediately while the capture-side tee ran in parallel.
Now, the entire RSC payload is buffered in memory before the response is created, blocking TTFB until the full render completes. For pages with <Suspense> and slow async server components, this could add seconds to TTFB.
This is a meaningful tradeoff — it's necessary to get consumeRenderResponseHeaders() to capture late async headers before the response is sent. But the HTML path solves this differently: it uses peekRenderResponseHeaders() for the live response and consumeRenderResponseHeaders() for the cache write, which runs in waitUntil. The RSC path could potentially use a similar pattern: stream to the client immediately using peek'd headers, then finalize the cache entry with consumed headers after the stream completes.
If the current approach is intentional (perhaps RSC responses are typically small enough that buffering is acceptable, or the correctness of final headers on the live response is deemed more important than streaming), please add a comment explaining the tradeoff.
P2: Dead code — __applyRenderResponseHeaders is defined but never called
__applyRenderResponseHeaders (line 359) wraps __mergeResponseHeaders(target, source, "fallback") but is never invoked anywhere in the generated entry. It's tested for existence in the code-gen assertions but has no callers. Should be removed or used.
P3: Headers iteration loses multi-value Set-Cookie headers
In __mergeResponseHeaders, the Headers branch iterates via for (const [key, value] of sourceHeaders). Per the Fetch spec, Headers.entries() joins multiple same-name header values with ", " — except Set-Cookie, which is exposed as separate entries only via Headers.getSetCookie(). However, for...of on a Headers object uses entries(), which will join multi-value Set-Cookie into one string. This means if sourceHeaders is a Headers instance that already has multiple Set-Cookie values appended to it, the merge path will flatten them into one concatenated value.
In practice this may not bite because render headers flow through the serialized Record<string, string | string[]> representation, not raw Headers. But the middleware _mwCtx.headers IS a Headers object that could have multiple Set-Cookie via append(). Worth checking whether the middleware merge path preserves cookie ordering correctly, or whether getSetCookie() should be used for the set-cookie key specifically.
P4: Vary append-only semantics can produce duplicate tokens
Because Vary is treated as append-only in __isAppendOnlyResponseHeader, replaying cached render headers that already contain "RSC, Accept" onto framework headers that also set "Vary: RSC, Accept" will produce Vary: RSC, Accept, RSC, Accept. While technically valid per HTTP spec (duplicates are harmless for Vary), it's cosmetically noisy and could confuse CDN operators reading response headers. A dedup step or a check before appending would be nice-to-have.
P5: __renderHeadersForCache fallback chain in HTML cache write path
Line 2916:
const __renderHeadersForCache = consumeRenderResponseHeaders() ?? renderResponseHeaders;This runs inside the waitUntil cache-write closure, after the HTML stream has been fully consumed. At this point, any late-async render work that ran during stream consumption may have produced new headers (captured by consumeRenderResponseHeaders()). The fallback to renderResponseHeaders (peek'd before stream consumption) handles the case where no new headers were added.
This is correct but subtle. A comment explaining why both are needed would help future maintainers.
P6: Background regen captures RSC data before consuming render headers
In the stale-while-revalidate background regen path (line ~2270):
const __freshRscData = await __rscDataPromise;
const __renderHeaders = consumeRenderResponseHeaders();The await __rscDataPromise was moved before consumeRenderResponseHeaders(). This is correct — the RSC stream tee must be fully consumed before headers are finalized (async components may append headers during streaming). But previously __freshRscData was awaited after the HTML collection loop. Moving it earlier means the HTML stream collection now starts after RSC data is fully captured, which shouldn't cause issues since the SSR stream is derived from the other tee branch and can buffer independently. Just noting this ordering change is intentional and sound.
Minor nits
-
The
__responseWithMiddlewareContextfunction conditionally preservesstatusTextonly when the status isn't overridden by a rewrite. This is a nice detail that avoids stale statusText (e.g., returning status 403 with statusText "OK"). -
Good that the
finallyblock in the RSC-first path (line 2697) ensures cleanup even if the cache write or response construction throws. -
The test for streaming behavior (
streams HTML ISR MISSes without waiting for full RSC capture) is a good regression guard. The tolerance thresholds look reasonable.
Overall this is solid work. The main item I'd want resolved before merge is the RSC-first MISS streaming regression (P1) — either the approach should be documented as intentional, or the HTML path's peek-for-response/consume-for-cache pattern should be applied to RSC too.
| __mergeResponseHeaderValues(targetHeaders, key, value, mode); | ||
| } | ||
| } | ||
| function __applyRenderResponseHeaders(targetHeaders, renderHeaders) { |
There was a problem hiding this comment.
Nit: __applyRenderResponseHeaders is defined here but never called anywhere in the generated entry. It's only tested for existence in entry-templates.test.ts. Consider removing it (dead code) or actually using it somewhere.
| function __mergeResponseHeaders(targetHeaders, sourceHeaders, mode) { | ||
| if (!sourceHeaders) return; | ||
| if (sourceHeaders instanceof Headers) { | ||
| for (const [key, value] of sourceHeaders) { |
There was a problem hiding this comment.
When sourceHeaders is a Headers instance, iterating with for...of uses entries() which joins multi-value headers with ", " per the Fetch spec. This is fine for most headers, but Set-Cookie has special semantics — entries() concatenates multiple Set-Cookie values into one string rather than yielding them separately (only getSetCookie() preserves them).
In the current code, this mostly doesn't matter because render headers arrive as Record<string, string | string[]> (the Object.entries branch). But _mwCtx.headers IS a Headers instance with potentially multiple Set-Cookie values appended via append(). If middleware sets multiple cookies, the merge here would flatten them into one concatenated string.
Consider adding a getSetCookie() special-case for the Headers branch, e.g.:
| for (const [key, value] of sourceHeaders) { | |
| if (sourceHeaders instanceof Headers) { | |
| for (const [key, value] of sourceHeaders) { | |
| if (key.toLowerCase() === "set-cookie") continue; | |
| __mergeResponseHeaderValues(targetHeaders, key, value, mode); | |
| } | |
| for (const cookie of sourceHeaders.getSetCookie()) { | |
| targetHeaders.append("Set-Cookie", cookie); | |
| } | |
| return; | |
| } |
| try { | ||
| const __rscDataForCache = await __isrRscDataPromise; | ||
| try { | ||
| const __rscDataForResponse = await __isrRscDataPromise; |
There was a problem hiding this comment.
Streaming regression for RSC-first MISS: Previously this path returned new Response(__rscForResponse, ...) where __rscForResponse was a live ReadableStream (the streaming half of the tee). Now it awaits the full __isrRscDataPromise to get the complete buffer before creating the response.
This means for RSC requests on ISR-eligible pages, the client must wait for the entire RSC payload to be generated before receiving any bytes — defeating streaming/progressive rendering for pages with suspended async components.
The HTML path handles this differently: it uses peekRenderResponseHeaders() for the live response and defers consumeRenderResponseHeaders() to the waitUntil cache-write closure. Could a similar approach work here?
// Conceptual sketch:
const __peakedHeaders = peekRenderResponseHeaders();
return __responseWithMiddlewareContext(
new Response(__rscForResponse, { status: 200, headers: __headersWithRenderResponseHeaders(responseHeaders, __peakedHeaders) }),
_mwCtx
);
// Then in waitUntil: await __isrRscDataPromise, consumeRenderResponseHeaders(), write cache
If the full-buffer approach is intentional (e.g., RSC payloads are small enough, or correctness of final headers on the live response is more important), please add a comment explaining the tradeoff.
| } | ||
| __chunks.push(__decoder.decode()); | ||
| const __fullHtml = __chunks.join(""); | ||
| const __renderHeadersForCache = consumeRenderResponseHeaders() ?? renderResponseHeaders; |
There was a problem hiding this comment.
This fallback chain is correct but subtle — consumeRenderResponseHeaders() captures any headers added by late async work during stream consumption, and falls back to renderResponseHeaders (peek'd before streaming started) if none were added. A brief comment explaining when each branch is used would help future readers:
| const __renderHeadersForCache = consumeRenderResponseHeaders() ?? renderResponseHeaders; | |
| const __renderHeadersForCache = consumeRenderResponseHeaders() ?? renderResponseHeaders; | |
| // ^ consume picks up any headers added during stream consumption by late | |
| // async render work (e.g. Suspense boundaries). Falls back to the snapshot | |
| // taken before streaming began if no new headers were produced. |
| } | ||
|
|
||
| if (renderResponseHeaders.setCookies.length > 0) { | ||
| serialized["set-cookie"] = renderResponseHeaders.setCookies.map((entry) => entry.value); |
There was a problem hiding this comment.
Minor: serializeRenderResponseHeaders uses the original casing from entry.name for non-cookie headers but always lowercases the key for set-cookie. This is correct (Set-Cookie is case-insensitive per spec), just noting the asymmetry is intentional since set-cookie is always stored via the setCookies array with a fixed lowercase key.
| } | ||
| const lowerName = name.toLowerCase(); | ||
| const existing = state.renderResponseHeaders.headers.get(lowerName); | ||
| if (existing) { |
There was a problem hiding this comment.
Nit: existing.name = name; unconditionally overwrites the stored header name with the latest casing on every append. This means if you do appendRenderResponseHeader("Vary", "a") then appendRenderResponseHeader("vary", "b"), the serialized key will be "vary" not "Vary". This probably doesn't matter (HTTP headers are case-insensitive), but if you want to preserve the original casing of the first append, consider not overwriting:
| if (existing) { | |
| if (existing) { | |
| existing.values.push(value); | |
| return; | |
| } |
|
Review posted successfully. Here's a summary of what I found: Review Summary for PR #488Overall: Well-structured change with sound architecture and thorough test coverage. The unified render-time header collector with lossless serialization and deterministic merge rules is a good design. Key findings:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Render-time response header cache parity
This is a well-designed change. The architecture — unified multi-value header collector in ALS → lossless serialization → deterministic merge on replay — is clean and correct. The test coverage is excellent, spanning MISS/HIT/STALE/regen-HIT for both HTML and RSC-first paths, plus edge cases for middleware merge, Vary, WWW-Authenticate, late async headers, and special responses.
The prior review from ask-bonk had some inaccurate findings that I want to correct:
-
P1 (RSC streaming regression) was incorrect. The RSC-first MISS path at line 2709 returns
new Response(__rscForResponse, ...)where__rscForResponseis still the live streaming half of the tee. The cache write runs inwaitUntilvia__rscWritePromise. The code usespeekRenderResponseHeaders()(line 2681) for the live response and defersconsumeRenderResponseHeaders()to the background cache write (line 2690) — exactly the same pattern as the HTML path. This was correctly tested by the "streams direct RSC ISR MISSes" test. -
P2 (dead code
__applyRenderResponseHeaders) was incorrect — that function doesn't exist. The actual functions are__applyMiddlewareResponseHeadersand__headersWithRenderResponseHeaders, both of which are used.
The items below are real findings from my review.
Findings
P3 (Middleware Set-Cookie flattening) — ask-bonk was correct about this one, though the PR already handles it with getSetCookie(). See my inline comment on the edge case where getSetCookie is unavailable.
Background regen ordering change — Moving await __rscDataPromise (line 2279) before the HTML stream collection loop is correct and intentional. The RSC stream must be fully consumed before consumeRenderResponseHeaders() can capture late async headers. The HTML stream is derived from the SSR entry processing the other tee branch, which buffers independently, so this reordering is safe.
Redirect handling — Changed from Response.redirect() to manual new Response(null, { status, headers: { Location } }) in handleRenderError. This is necessary because Response.redirect() creates an immutable-headers response, making it impossible to merge render-time or middleware headers onto it afterward. Good change.
Overall, this is solid work with thorough testing. The main items are the two inline comments below.
| : []; | ||
| for (const [key, value] of sourceHeaders) { | ||
| if (key.toLowerCase() === "set-cookie") { | ||
| if (__setCookies.length === 0) { |
There was a problem hiding this comment.
The getSetCookie() guard on line 350 handles the common case, but when getSetCookie is unavailable (the __setCookies.length === 0 fallback at line 355-357), Set-Cookie values from a Headers instance will be flattened into a single comma-joined string by entries(). This means the fallback path produces incorrect Set-Cookie values.
In practice, getSetCookie() is available in all modern runtimes and Cloudflare Workers, so this may never trigger. But if the fallback is meant to be a real safety net, it would need to handle Set-Cookie separately (e.g., skip it entirely or split on , with a cookie-aware parser). As-is, the fallback silently produces broken cookies.
Consider either:
- Removing the fallback entirely and trusting
getSetCookie()exists (simplest) - Adding a comment that the fallback produces degraded
Set-Cookiehandling - Skipping
Set-Cookiein the fallback (losing cookies is better than merging broken ones)
| } | ||
| const lowerName = name.toLowerCase(); | ||
| const existing = state.renderResponseHeaders.headers.get(lowerName); | ||
| if (existing) { |
There was a problem hiding this comment.
Nit: existing.values.push(value) is correct, but the name casing stored in existing.name reflects whichever casing was used on the first append() call. If a later append uses different casing (e.g., appendRenderResponseHeader("vary", "b") after appendRenderResponseHeader("Vary", "a")), the serialized key will be "Vary" — the original casing is preserved.
This is actually the better behavior (preserve first-seen casing), and it already works correctly because of the early return here. The previous review's suggestion to remove existing.name = name was based on an earlier revision that had that overwrite — this version doesn't have it. No action needed, just confirming the current code is correct.
| for (const item of values) targetHeaders.append(key, item); | ||
| return; | ||
| } | ||
| if (mode === "fallback" && targetHeaders.has(key)) return; |
There was a problem hiding this comment.
Subtle case-sensitivity issue: targetHeaders.has(key) on a Headers instance is case-insensitive per spec, but when value comes from a Record<string, string | string[]> (the Object.entries branch), the key preserves original casing from serialization (e.g., "Vary", "x-rendered"). This works correctly because Headers.has() is case-insensitive.
However, if mode is "fallback" and the target already has the key under different casing (e.g., target has "vary", source has "Vary"), the fallback correctly skips — but a subsequent non-fallback merge with "override" mode would delete(key) then set(key, ...), which correctly replaces regardless of casing.
No bug here, just noting the case-insensitivity of Headers is load-bearing for correctness.
| const __revalSsrEntry = await import.meta.viteRsc.loadModule("ssr", "index"); | ||
| const __revalHtmlStream = await __revalSsrEntry.handleSsr(__revalRscForSsr, _getNavigationContext(), __revalFontData); | ||
| const __freshRscData = await __rscDataPromise; | ||
| const __renderHeaders = consumeRenderResponseHeaders(); |
There was a problem hiding this comment.
Good ordering change. Awaiting __rscDataPromise before consumeRenderResponseHeaders() is necessary because async server components running during RSC stream generation may append headers. The HTML stream collection that follows (lines 2284-2293) is safe to run after because the SSR stream was already created from the other tee branch at line 2278 and buffers independently.
A brief comment here would help future readers understand why the RSC data must be fully consumed before headers are captured:
| const __renderHeaders = consumeRenderResponseHeaders(); | |
| const __freshRscData = await __rscDataPromise; | |
| // RSC stream must be fully consumed before capturing render headers, | |
| // since async server components may append headers during streaming. | |
| const __renderHeaders = consumeRenderResponseHeaders(); |
| @@ -575,8 +674,13 @@ export function cookies(): Promise<RequestCookies> & RequestCookies { | |||
| */ | |||
| export function getAndClearPendingCookies(): string[] { | |||
There was a problem hiding this comment.
Both getAndClearPendingCookies and getDraftModeCookieHeader are no longer imported by the generated RSC entry — they've been fully replaced by consumeRenderResponseHeaders(). They're still exported and tested in shims.test.ts, but are they needed by any other consumer? If they're only kept for backward compatibility with user code that might import them directly, consider marking them as deprecated. If they're purely internal, they could be removed to reduce the API surface.
|
Review submitted successfully. Here's a summary of what I posted: Overall assessment: Well-designed change with clean architecture and thorough test coverage. The unified render-time header collector with lossless serialization and deterministic merge rules is sound. Corrections to prior review:
Real findings:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: Render-time response header cache parity
This is well-architected work. The unified render-time header collector in ALS → lossless multi-value serialization → deterministic merge-on-replay design is clean and correct. The test coverage is thorough: MISS/HIT/STALE/regen-HIT for both HTML and RSC paths, plus edge cases for middleware merge, Vary, WWW-Authenticate, late async headers, redirects, layout notFound, and metadata resolution. The refactoring of handleRenderError to a single shared helper and the change from Response.redirect() to manual new Response(null, { status, headers: { Location } }) to allow header merging are both good improvements.
Previous reviews incorrectly flagged the RSC-first MISS path as a streaming regression. The code at line 2708 correctly returns new Response(__rscForResponse, ...) where __rscForResponse is the live streaming tee branch; the await __isrRscDataPromise + consumeRenderResponseHeaders() runs in the background __rscWritePromise. The streaming tests at lines 1845-1888 confirm this.
I have one substantive finding and a few minor items below.
Summary
1 real issue (P2): Non-ISR RSC responses don't get render-time response headers applied.
3 minor items: Missing cleanup in non-ISR RSC finally, getSetCookie fallback could be simplified, and getAndClearPendingCookies/getDraftModeCookieHeader deprecation.
Overall this is solid and close to merge-ready.
| return __responseWithMiddlewareContext(new Response(__rscForResponse, { | ||
| status: 200, | ||
| headers: responseHeaders, | ||
| }), _mwCtx); |
There was a problem hiding this comment.
P2: Non-ISR RSC responses don't receive render-time response headers
The ISR RSC MISS path (line 2708-2711) correctly passes __responseRenderHeaders into __headersWithRenderResponseHeaders. But the non-ISR fallback here only passes responseHeaders (the plain object) without any render-time headers. This means for RSC responses on non-ISR pages (e.g., revalidate not set, force-dynamic, dev mode), any render-time headers set via appendRenderResponseHeader during buildPageElement / generateMetadata will be silently dropped.
This is the same path used for all dev-mode RSC requests (since __isrRscDataPromise is null in dev), so the render-time header APIs won't work for RSC client-side navigation during development.
Note that the RSC stream is consumed lazily, so headers set during async streaming work won't be captured anyway — but synchronous headers set in buildPageElement / generateMetadata (snapshot'd into __buildRenderResponseHeaders and restored at line 2589) should be available here.
| }), _mwCtx); | |
| return __responseWithMiddlewareContext(new Response(__rscForResponse, { | |
| status: 200, | |
| headers: __headersWithRenderResponseHeaders(responseHeaders, peekRenderResponseHeaders()), | |
| }), _mwCtx); |
Or, since there's no waitUntil cache-write to defer to, consumeRenderResponseHeaders() could be used instead of peek (matching the pattern elsewhere). The headers context will be cleaned up when the ALS scope ends.
| const __setCookies = typeof sourceHeaders.getSetCookie === "function" | ||
| ? sourceHeaders.getSetCookie() | ||
| : []; | ||
| for (const [key, value] of sourceHeaders) { |
There was a problem hiding this comment.
Minor: The getSetCookie unavailability path now correctly skips cookies with continue instead of corrupting them — good change from the earlier revision. But the guard typeof sourceHeaders.getSetCookie === "function" + empty array fallback means if getSetCookie is unavailable, middleware Set-Cookie values are silently dropped entirely.
Since getSetCookie() is available in all target runtimes (Cloudflare Workers, Node 20+), consider simplifying by removing the guard and calling getSetCookie() unconditionally. This would make the contract explicit: vinext requires a runtime with Headers.getSetCookie() support. If there's a reason to support older runtimes, a comment explaining the silent cookie drop would help.
| const __revalHtmlStream = await __revalSsrEntry.handleSsr(__revalRscForSsr, _getNavigationContext(), __revalFontData); | ||
| const __freshRscData = await __rscDataPromise; | ||
| const __renderHeaders = consumeRenderResponseHeaders(); | ||
| setHeadersContext(null); |
There was a problem hiding this comment.
Agreed with the earlier ask-bonk review: this ordering change is correct and important, but subtle enough to warrant a comment.
| const __revalHtmlStream = await __revalSsrEntry.handleSsr(__revalRscForSsr, _getNavigationContext(), __revalFontData); | |
| const __freshRscData = await __rscDataPromise; | |
| const __renderHeaders = consumeRenderResponseHeaders(); | |
| setHeadersContext(null); | |
| const __freshRscData = await __rscDataPromise; | |
| // RSC stream must be fully consumed before capturing render headers, | |
| // since async server components may append headers during streaming. | |
| const __renderHeaders = consumeRenderResponseHeaders(); |
| @@ -575,8 +674,13 @@ export function cookies(): Promise<RequestCookies> & RequestCookies { | |||
| */ | |||
| export function getAndClearPendingCookies(): string[] { | |||
There was a problem hiding this comment.
Both getAndClearPendingCookies and getDraftModeCookieHeader are no longer used by the generated RSC entry — all call sites have been replaced with consumeRenderResponseHeaders(). They're still exported and tested in shims.test.ts, and getAndClearPendingCookies is still tested indirectly (the "keeps cookie helper queues separate" test).
If these are only kept for backward compatibility (e.g., user code or third-party libraries importing them directly), consider marking them @deprecated with a note pointing to the new API. If they're purely internal, they could be removed in a follow-up to shrink the API surface.
Not blocking — just a cleanup opportunity.
| function serializeRenderResponseHeaders( | ||
| renderResponseHeaders: RenderResponseHeaders, | ||
| ): Record<string, string | string[]> | undefined { | ||
| const serialized: Record<string, string | string[]> = {}; | ||
|
|
||
| for (const entry of renderResponseHeaders.headers.values()) { | ||
| if (entry.values.length === 1) { | ||
| serialized[entry.name] = entry.values[0]!; | ||
| continue; | ||
| } | ||
| if (entry.values.length > 1) { | ||
| serialized[entry.name] = [...entry.values]; | ||
| } | ||
| } | ||
|
|
||
| if (renderResponseHeaders.setCookies.length > 0) { | ||
| serialized["set-cookie"] = renderResponseHeaders.setCookies.map((entry) => entry.value); | ||
| } | ||
|
|
||
| return Object.keys(serialized).length > 0 ? serialized : undefined; |
There was a problem hiding this comment.
Minor note: serializeRenderResponseHeaders allocates a serialized object and iterates all entries even when the render response headers are empty (which is the common case for most pages that don't call appendRenderResponseHeader). An early return when both headers and setCookies are empty would avoid the allocation:
| function serializeRenderResponseHeaders( | |
| renderResponseHeaders: RenderResponseHeaders, | |
| ): Record<string, string | string[]> | undefined { | |
| const serialized: Record<string, string | string[]> = {}; | |
| for (const entry of renderResponseHeaders.headers.values()) { | |
| if (entry.values.length === 1) { | |
| serialized[entry.name] = entry.values[0]!; | |
| continue; | |
| } | |
| if (entry.values.length > 1) { | |
| serialized[entry.name] = [...entry.values]; | |
| } | |
| } | |
| if (renderResponseHeaders.setCookies.length > 0) { | |
| serialized["set-cookie"] = renderResponseHeaders.setCookies.map((entry) => entry.value); | |
| } | |
| return Object.keys(serialized).length > 0 ? serialized : undefined; | |
| function serializeRenderResponseHeaders( | |
| renderResponseHeaders: RenderResponseHeaders, | |
| ): Record<string, string | string[]> | undefined { | |
| if (renderResponseHeaders.headers.size === 0 && renderResponseHeaders.setCookies.length === 0) { | |
| return undefined; | |
| } | |
| const serialized: Record<string, string | string[]> = {}; | |
| for (const entry of renderResponseHeaders.headers.values()) { | |
| if (entry.values.length === 1) { | |
| serialized[entry.name] = entry.values[0]!; | |
| continue; | |
| } | |
| if (entry.values.length > 1) { | |
| serialized[entry.name] = [...entry.values]; | |
| } | |
| } | |
| if (renderResponseHeaders.setCookies.length > 0) { | |
| serialized["set-cookie"] = renderResponseHeaders.setCookies.map((entry) => entry.value); | |
| } | |
| return Object.keys(serialized).length > 0 ? serialized : undefined; | |
| } |
This is a hot path — peek and consume are called multiple times per request. The early return avoids object allocation and iteration when there are no render headers (most requests).
|
Review posted successfully. Here's a summary of what I found: Review Summary for PR #488Overall assessment: Well-architected change with clean design and thorough test coverage. The unified render-time header collector → lossless serialization → deterministic merge pattern is sound. Key correction: Previous reviews incorrectly flagged the RSC-first MISS path as a streaming regression. The code correctly uses Findings
|
2 participants
Bring cached App Router response-header behavior closer to Next.js by preserving and replaying render-time response headers losslessly.
This fixes parity for headers that are mutated during render rather than after the final
Responseis created, including:VarySet-CookieWWW-Authenticate/Proxy-AuthenticateWhat changed
Headers, so repeated non-cookie headers are not flattened before cachingset-cookie: append and preserve ordervary: append/merge, never overwritewww-authenticate/proxy-authenticate: append repeated valuesHITandSTALEresponsesMISSresponsesTests
Added and updated coverage for:
MISS/HIT/STALE/ post-regenerationHITMISS/HITVaryreplay across cached responsesWWW-Authenticatereplay/merge behaviorSet-CookiereplayVary, and cookie merge behavior on cached responsesnotFound()special-response handlingcookies()anddraftMode()serialization into render response headersValidation
Ran:
pnpm test tests/shims.test.ts tests/isr-cache.test.ts tests/app-router.test.ts tests/entry-templates.test.ts -upnpm run fmtpnpm run typecheckNotes
This does not special-case cookies. The cached artifact is a general render-time response-header payload with lossless multi-value handling for repeated headers.