fix(app-router): emit vary marker on edge RSC route response#1565
fix(app-router): emit vary marker on edge RSC route response#1565james-elicx wants to merge 1 commit into
Conversation
Closes #1531 The Next.js edge-runtime app template (edge-ssr-app.ts) sets `x-edge-runtime: 1` on every App Router response. vinext serves App Router responses from a Cloudflare Worker (an edge-equivalent runtime) but was not emitting this marker, so Next.js' upstream test suite (`test/e2e/app-dir/app/index.test.ts`) saw `null` for the header. Add the header to both the RSC and HTML response builders so the assertion passes for any App Router page, regardless of which payload shape the client requests.
|
/bigbonk review |
commit: |
|
There was a problem hiding this comment.
Review
The implementation is correct for the stated goal: adding x-edge-runtime: 1 to the two buildAppPage*Response helpers so the ported Next.js test assertion passes. Tests are well-structured and the comments citing edge-ssr-app.ts are helpful.
Two things worth calling out:
1. Incomplete coverage across App Router response paths
buildAppPageRscResponse and buildAppPageHtmlResponse are only two of many App Router response construction sites. Other paths that build RSC/HTML responses independently (and therefore won't carry this header) include:
app-page-cache.ts—buildAppPageCachedHeaders()(ISR HIT/STALE responses)app-server-action-execution.ts— server action RSC responses (no-revalidation, re-render, redirect)app-page-dispatch.ts— interception route responsesapp-page-boundary.ts— boundary fallback RSC responsesapp-page-execution.ts— RSC redirect responsesapp-page-stream.ts—renderAppPageHtmlResponse()(separate HTML response path)app-route-handler-response.ts— route handler responses
If the intent is "every App Router response from vinext should carry x-edge-runtime: 1" (since vinext always runs on Workers), these other paths are gaps. Probably fine as a follow-up since this PR is scoped to the specific test failure, but worth a tracking issue.
2. Nuance on what Next.js actually does
In Next.js, x-edge-runtime: 1 is set inside the edge-ssr-app.ts template — it only applies to pages that opt into runtime = 'edge'. Standard Node.js App Router pages do not get this header. vinext's choice to emit it unconditionally (since all responses are edge-equivalent on Workers) is a reasonable divergence, and the comment in the code explains the rationale well. Just flagging it as context.
Verdict
Looks good. The fix is minimal, targeted, and correctly addresses the test failure from #1531.
|
Review posted on PR #1565. Here's a summary: The fix is correct and well-scoped. It adds I flagged two observations in the review:
|
Closes #1531
Summary
x-edge-runtime: 1marker header on edge runtime App Router responses (both RSC and HTML response builders) to match Next.js, which sets this header inedge-ssr-app.ts.test/e2e/app-dir/app/index.test.ts"should return thevaryheader from edge runtime" assertion fails — it sawnullinstead of'1'.Test plan
tests/app-page-response.test.tsassert the header is present on both RSC and HTML responses.pnpm run checkpasses (lint, format, types).app-page-response,app-page-render, andapp-routersuites stay green.