-
Notifications
You must be signed in to change notification settings - Fork 306
feat(app-router): filter skipped layouts from RSC responses and cached reads [3/6] #840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import type { CachedAppPageValue } from "../shims/cache.js"; | ||
| import { buildAppPageCacheValue, type ISRCacheEntry } from "./isr-cache.js"; | ||
| import { wrapRscBytesForResponse } from "./app-page-skip-filter.js"; | ||
|
|
||
| const EMPTY_SKIP_SET: ReadonlySet<string> = new Set<string>(); | ||
|
|
||
| type AppPageDebugLogger = (event: string, detail: string) => void; | ||
| type AppPageCacheGetter = (key: string) => Promise<ISRCacheEntry | null>; | ||
|
|
@@ -22,6 +25,7 @@ export type BuildAppPageCachedResponseOptions = { | |
| isRscRequest: boolean; | ||
| mountedSlotsHeader?: string | null; | ||
| revalidateSeconds: number; | ||
| skipIds?: ReadonlySet<string>; | ||
| }; | ||
|
|
||
| export type ReadAppPageCacheResponseOptions = { | ||
|
|
@@ -37,6 +41,7 @@ export type ReadAppPageCacheResponseOptions = { | |
| revalidateSeconds: number; | ||
| renderFreshPageForCache: () => Promise<AppPageCacheRenderResult>; | ||
| scheduleBackgroundRegeneration: AppPageBackgroundRegenerator; | ||
| skipIds?: ReadonlySet<string>; | ||
| }; | ||
|
|
||
| export type FinalizeAppPageHtmlCacheResponseOptions = { | ||
|
|
@@ -106,7 +111,11 @@ export function buildAppPageCachedResponse( | |
| rscHeaders["X-Vinext-Mounted-Slots"] = options.mountedSlotsHeader; | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good use of |
||
| return new Response(cachedValue.rscData, { | ||
| // Cached bytes stay canonical. The current request's skipIds decide | ||
| // whether this client gets the full payload or a filtered egress view. | ||
| const body = wrapRscBytesForResponse(cachedValue.rscData, options.skipIds ?? EMPTY_SKIP_SET); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cache-read path applies
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documented. Added an inline comment on the cache-read path clarifying that the cached bytes stay canonical and the current request’s |
||
|
|
||
| return new Response(body, { | ||
| status, | ||
| headers: rscHeaders, | ||
| }); | ||
|
|
@@ -142,6 +151,7 @@ export async function readAppPageCacheResponse( | |
| isRscRequest: options.isRscRequest, | ||
| mountedSlotsHeader: options.mountedSlotsHeader, | ||
| revalidateSeconds: options.revalidateSeconds, | ||
| skipIds: options.skipIds, | ||
| }); | ||
|
|
||
| if (hitResponse) { | ||
|
|
@@ -198,6 +208,7 @@ export async function readAppPageCacheResponse( | |
| isRscRequest: options.isRscRequest, | ||
| mountedSlotsHeader: options.mountedSlotsHeader, | ||
| revalidateSeconds: options.revalidateSeconds, | ||
| skipIds: options.skipIds, | ||
| }); | ||
|
|
||
| if (staleResponse) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,15 @@ | ||
| import type { ReactNode } from "react"; | ||
| import type { CachedAppPageValue } from "../shims/cache.js"; | ||
| import { buildOutgoingAppPayload, type AppOutgoingElements } from "./app-elements.js"; | ||
| import { | ||
| buildOutgoingAppPayload, | ||
| computeSkipDecision, | ||
| type AppOutgoingElements, | ||
| } from "./app-elements.js"; | ||
| import { | ||
| finalizeAppPageHtmlCacheResponse, | ||
| scheduleAppPageRscCacheWrite, | ||
| } from "./app-page-cache.js"; | ||
| import { createSkipFilterTransform } from "./app-page-skip-filter.js"; | ||
| import { | ||
| buildAppPageFontLinkHeader, | ||
| resolveAppPageSpecialError, | ||
|
|
@@ -32,6 +37,8 @@ import { | |
| type AppPageSsrHandler, | ||
| } from "./app-page-stream.js"; | ||
|
|
||
| const EMPTY_SKIP_SET: ReadonlySet<string> = new Set<string>(); | ||
|
|
||
| type AppPageBoundaryOnError = ( | ||
| error: unknown, | ||
| requestInfo: unknown, | ||
|
|
@@ -68,6 +75,7 @@ export type RenderAppPageLifecycleOptions = { | |
| isForceStatic: boolean; | ||
| isProduction: boolean; | ||
| isRscRequest: boolean; | ||
| supportsFilteredRscStream?: boolean; | ||
| isrDebug?: AppPageDebugLogger; | ||
| isrHtmlKey: (pathname: string) => string; | ||
| isrRscKey: (pathname: string, mountedSlotsHeader?: string | null) => string; | ||
|
|
@@ -97,6 +105,7 @@ export type RenderAppPageLifecycleOptions = { | |
| waitUntil?: (promise: Promise<void>) => void; | ||
| element: ReactNode | Readonly<Record<string, ReactNode>>; | ||
| classification?: LayoutClassificationOptions | null; | ||
| requestedSkipLayoutIds?: ReadonlySet<string>; | ||
| }; | ||
|
|
||
| function buildResponseTiming( | ||
|
|
@@ -148,9 +157,9 @@ export async function renderAppPageLifecycle( | |
|
|
||
| const layoutFlags = preRenderResult.layoutFlags; | ||
|
|
||
| // Render the CANONICAL element. The outgoing payload carries per-layout | ||
| // static/dynamic flags under `__layoutFlags` so the client can later tell | ||
| // which layouts are safe to skip on subsequent navigations. | ||
| // Always render the CANONICAL element. Skip semantics are applied on the | ||
| // egress branch only so the cache branch receives full bytes regardless of | ||
| // the client's skip header. See `app-page-skip-filter.ts`. | ||
| const outgoingElement = buildOutgoingAppPayload({ | ||
| element: options.element, | ||
| layoutFlags, | ||
|
|
@@ -172,9 +181,17 @@ export async function renderAppPageLifecycle( | |
| revalidateSeconds !== Infinity && | ||
| !options.isForceDynamic, | ||
| ); | ||
| const rscForResponse = rscCapture.responseStream; | ||
| const isrRscDataPromise = rscCapture.capturedRscDataPromise; | ||
|
|
||
| const skipIds = | ||
| options.isRscRequest && (options.supportsFilteredRscStream ?? false) | ||
| ? computeSkipDecision(layoutFlags, options.requestedSkipLayoutIds) | ||
| : EMPTY_SKIP_SET; | ||
| const rscForResponse = | ||
| skipIds.size > 0 | ||
| ? rscCapture.responseStream.pipeThrough(createSkipFilterTransform(skipIds)) | ||
| : rscCapture.responseStream; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth noting for future readers:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an inline comment at the HTML SSR call site clarifying that
Comment on lines
+190
to
+193
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So from what I gather, it sounds like we render the entire tree, then skip returning parts of that tree to client? Wouldn't we want to skip rendering those before-hand to reduce server work? I believe both Next.js and Waku would skip before the rendering. |
||
|
|
||
| if (options.isRscRequest) { | ||
| const dynamicUsedDuringBuild = options.consumeDynamicUsage(); | ||
| const rscResponsePolicy = resolveAppPageRscResponsePolicy({ | ||
|
|
@@ -241,6 +258,9 @@ export async function renderAppPageLifecycle( | |
| return renderAppPageHtmlStream({ | ||
| fontData, | ||
| navigationContext: options.getNavigationContext(), | ||
| // HTML SSR always consumes the canonical stream: skipIds is only | ||
| // non-empty for RSC requests, so the HTML path never sees filtered | ||
| // bytes even though it reuses the same `rscForResponse` handle. | ||
| rscStream: rscForResponse, | ||
| scriptNonce: options.scriptNonce, | ||
| ssrHandler, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header parsing accepts any
layout:*prefixed value without length or character validation. A malicious client could send a very long header with many entries to force the server to allocate a largeSet. Since this runs on every RSC request (when the header is present), consider adding a cap — e.g., bail out or truncate after N entries (say 50, since deeply nested layout trees beyond that would be unusual).This is low severity since Cloudflare Workers already enforce request header size limits, but defense-in-depth at the application layer doesn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
parseSkipHeadernow uses a bounded parser and stops after 50 uniquelayout:*ids, which keeps the allocation surface capped even if a client sends a pathological skip header. Added a test for the cap.