-
Notifications
You must be signed in to change notification settings - Fork 181
Implement PPR support in cache interceptor #1057
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
base: adapters-api
Are you sure you want to change the base?
Changes from all commits
e3850e4
e2572a3
fa92ae4
31a6d9a
715e05e
9f7cb21
a2e94ff
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 |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@opennextjs/aws": minor | ||
| --- | ||
|
|
||
| Make PPR work with the cache interceptor | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import type { | |
| InternalEvent, | ||
| InternalResult, | ||
| MiddlewareEvent, | ||
| PartialResult, | ||
| } from "types/open-next"; | ||
| import type { CacheValue } from "types/overrides"; | ||
| import { emptyReadableStream, toReadableStream } from "utils/stream"; | ||
|
|
@@ -134,37 +135,107 @@ function getBodyForAppRouter( | |
| } | ||
| } | ||
|
|
||
| function createPprPartialResult( | ||
| event: MiddlewareEvent, | ||
| localizedPath: string, | ||
| cachedValue: CacheValue<"cache">, | ||
| responseBody: string | (() => ReturnType<typeof toReadableStream>), | ||
| contentType: string, | ||
| ): PartialResult { | ||
| if (cachedValue.type !== "app") { | ||
| throw new Error("createPprPartialResult called with non-app cache value"); | ||
| } | ||
|
|
||
| return { | ||
| resumeRequest: { | ||
| ...event, | ||
| method: "POST", | ||
| url: `http://${event.headers.host}${NextConfig.basePath || ""}${ | ||
| localizedPath || "/" | ||
| }`, | ||
| headers: { | ||
| ...event.headers, | ||
| "next-resume": "1", | ||
| }, | ||
| rawPath: localizedPath, | ||
| body: Buffer.from(cachedValue.meta?.postponed || "", "utf-8"), | ||
| }, | ||
| result: { | ||
| type: "core", | ||
| statusCode: event.rewriteStatusCode ?? cachedValue.meta?.status ?? 200, | ||
| body: | ||
| typeof responseBody === "string" | ||
| ? toReadableStream(responseBody) | ||
| : responseBody(), | ||
| isBase64Encoded: false, | ||
| headers: { | ||
| "content-type": contentType, | ||
| "x-opennext-ppr": "1", | ||
| ...cachedValue.meta?.headers, | ||
| vary: VARY_HEADER, | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| async function generateResult( | ||
| event: MiddlewareEvent, | ||
| localizedPath: string, | ||
| cachedValue: CacheValue<"cache">, | ||
| lastModified?: number, | ||
| ): Promise<InternalResult> { | ||
| ): Promise<InternalResult | PartialResult | InternalEvent> { | ||
| debug("Returning result from experimental cache"); | ||
| let body = ""; | ||
| let type = "application/octet-stream"; | ||
| let isDataRequest = false; | ||
| let additionalHeaders = {}; | ||
| let body: string; | ||
| if (cachedValue.type === "app") { | ||
| isDataRequest = Boolean(event.headers.rsc); | ||
| if (isDataRequest) { | ||
| const { body: appRouterBody, additionalHeaders: appHeaders } = | ||
| getBodyForAppRouter(event, cachedValue); | ||
| body = appRouterBody; | ||
| additionalHeaders = appHeaders; | ||
| if (cachedValue.meta?.postponed) { | ||
| debug("App router postponed request detected", localizedPath); | ||
| return createPprPartialResult( | ||
| event, | ||
| localizedPath, | ||
| cachedValue, | ||
| () => emptyReadableStream(), | ||
| "text/x-component", | ||
| ); | ||
| } | ||
| debug("App router data request detected", localizedPath, body); | ||
| } else { | ||
| body = cachedValue.html; | ||
| if (cachedValue.meta?.postponed) { | ||
| debug("Postponed request detected", localizedPath); | ||
| return createPprPartialResult( | ||
| event, | ||
| localizedPath, | ||
| cachedValue, | ||
| cachedValue.html, | ||
| "text/html; charset=utf-8", | ||
| ); | ||
| } else { | ||
| body = cachedValue.html; | ||
| } | ||
| } | ||
| type = isDataRequest ? "text/x-component" : "text/html; charset=utf-8"; | ||
| } else if (cachedValue.type === "page") { | ||
| isDataRequest = Boolean(event.query.__nextDataReq); | ||
| body = isDataRequest ? JSON.stringify(cachedValue.json) : cachedValue.html; | ||
| if (isDataRequest) { | ||
|
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. if we want to change the ternary to an if then it would make sense to fold l 232 here as well? |
||
| body = JSON.stringify(cachedValue.json); | ||
| } else { | ||
| body = cachedValue.html; | ||
| } | ||
| type = isDataRequest ? "application/json" : "text/html; charset=utf-8"; | ||
| } else { | ||
| throw new Error( | ||
| "generateResult called with unsupported cache value type, only 'app' and 'page' are supported", | ||
| ); | ||
| } | ||
| // close(); | ||
|
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. What to do with this?
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. I'll remove it |
||
| const cacheControl = await computeCacheControl( | ||
| localizedPath, | ||
| body, | ||
|
|
@@ -180,7 +251,7 @@ async function generateResult( | |
| // `NextResponse.rewrite(url, { status: xxx}) | ||
| // The rewrite status code should take precedence over the cached one | ||
| statusCode: event.rewriteStatusCode ?? cachedValue.meta?.status ?? 200, | ||
| body: toReadableStream(body, false), | ||
| body: toReadableStream(body, isBinaryContentType(type)), | ||
| isBase64Encoded: false, | ||
| headers: { | ||
| ...cacheControl, | ||
|
|
@@ -227,13 +298,20 @@ function decodePathParams(pathname: string): string { | |
|
|
||
| export async function cacheInterceptor( | ||
| event: MiddlewareEvent, | ||
| ): Promise<InternalEvent | InternalResult> { | ||
| ): Promise<InternalEvent | InternalResult | PartialResult> { | ||
| if ( | ||
| Boolean(event.headers["next-action"]) || | ||
| Boolean(event.headers["x-prerender-revalidate"]) | ||
| Boolean(event.headers["x-prerender-revalidate"]) || | ||
| Boolean(event.headers["next-resume"]) || | ||
| event.method !== "GET" | ||
| ) | ||
| return event; | ||
|
|
||
| // if(Boolean(event.headers.rsc) && !(Boolean(event.headers["next-router-prefetch"]) || Boolean(event.headers[NEXT_SEGMENT_PREFETCH_HEADER]))) { | ||
| // // Let the handler deal with RSC requests with no prefetch header as they are SSR requests | ||
| // return event; | ||
| // } | ||
|
|
||
| // Check for Next.js preview mode cookies | ||
| const cookies = event.headers.cookie || ""; | ||
| const hasPreviewData = | ||
|
|
@@ -258,19 +336,35 @@ export async function cacheInterceptor( | |
|
|
||
| debug("Checking cache for", localizedPath, PrerenderManifest); | ||
|
|
||
| const isISR = | ||
| Object.keys(PrerenderManifest?.routes ?? {}).includes( | ||
| localizedPath ?? "/", | ||
| ) || | ||
| Object.values(PrerenderManifest?.dynamicRoutes ?? {}).some((dr) => | ||
| new RegExp(dr.routeRegex).test(localizedPath), | ||
| ); | ||
| const isDynamicISR = Object.values( | ||
| PrerenderManifest?.dynamicRoutes ?? {}, | ||
| ).some((dr) => { | ||
| const regex = new RegExp(dr.routeRegex); | ||
| return regex.test(localizedPath); | ||
| }); | ||
|
|
||
| const isStaticRoute = Object.keys(PrerenderManifest?.routes ?? {}).includes( | ||
| localizedPath || "/", | ||
| ); | ||
|
|
||
| const isISR = isStaticRoute || isDynamicISR; | ||
| debug("isISR", isISR); | ||
| if (isISR) { | ||
| try { | ||
| const cachedData = await globalThis.incrementalCache.get( | ||
| localizedPath ?? "/index", | ||
| ); | ||
| let pathToUse = localizedPath; | ||
| // For PPR, we need to check the fallback value to get the correct cache key | ||
| // We don't want to override a static route though | ||
| if (isDynamicISR && !isStaticRoute) { | ||
| pathToUse = Object.entries(PrerenderManifest?.dynamicRoutes ?? {}).find( | ||
| ([, dr]) => { | ||
| const regex = new RegExp(dr.routeRegex); | ||
| return regex.test(localizedPath); | ||
| }, | ||
| )?.[1].fallback! as string; | ||
| } else if (localizedPath === "") { | ||
| pathToUse = "/index"; | ||
| } | ||
| const cachedData = await globalThis.incrementalCache.get(pathToUse); | ||
| debug("cached data in interceptor", cachedData); | ||
|
|
||
| if (!cachedData?.value) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { | |
| import type { | ||
| InternalEvent, | ||
| InternalResult, | ||
| PartialResult, | ||
| ResolvedRoute, | ||
| RoutingResult, | ||
| } from "types/open-next"; | ||
|
|
@@ -235,26 +236,43 @@ export default async function routingHandler( | |
| }; | ||
| } | ||
|
|
||
| const resolvedRoutes: ResolvedRoute[] = [ | ||
| ...foundStaticRoute, | ||
| ...foundDynamicRoute, | ||
| ]; | ||
|
|
||
| if ( | ||
| globalThis.openNextConfig.dangerous?.enableCacheInterception && | ||
| !isInternalResult(eventOrResult) | ||
| ) { | ||
| debug("Cache interception enabled"); | ||
| eventOrResult = await cacheInterceptor(eventOrResult); | ||
| if (isInternalResult(eventOrResult)) { | ||
| applyMiddlewareHeaders(eventOrResult, headers); | ||
| return eventOrResult; | ||
| const cacheInterceptionResult = await cacheInterceptor(eventOrResult); | ||
| if (isInternalResult(cacheInterceptionResult)) { | ||
| applyMiddlewareHeaders(cacheInterceptionResult, headers); | ||
|
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. q: would it make sense for
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. Yeah, I guess. I'll have to check everywhere where it's used, but yeah |
||
| return cacheInterceptionResult; | ||
| } else if (isPartialResult(cacheInterceptionResult)) { | ||
|
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. nit: no else after return?
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. It's an else if, there is a third case that none of these 2 are supposed to catch (when it is an InternalEvent) |
||
| // We need to apply the headers to both the result (the streamed response) and the resume request | ||
| applyMiddlewareHeaders(cacheInterceptionResult.result, headers); | ||
| applyMiddlewareHeaders(cacheInterceptionResult.resumeRequest, headers); | ||
| return { | ||
| internalEvent: cacheInterceptionResult.resumeRequest, | ||
| isExternalRewrite: false, | ||
| origin: false, | ||
| isISR: false, | ||
| resolvedRoutes, | ||
| initialURL: event.url, | ||
| locale: NextConfig.i18n | ||
| ? detectLocale(eventOrResult, NextConfig.i18n) | ||
| : undefined, | ||
| rewriteStatusCode: middlewareEventOrResult.rewriteStatusCode, | ||
| initialResponse: cacheInterceptionResult.result, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // We apply the headers from the middleware response last | ||
| applyMiddlewareHeaders(eventOrResult, headers); | ||
|
|
||
| const resolvedRoutes: ResolvedRoute[] = [ | ||
| ...foundStaticRoute, | ||
| ...foundDynamicRoute, | ||
| ]; | ||
|
|
||
| debug("resolvedRoutes", resolvedRoutes); | ||
|
|
||
| return { | ||
|
|
@@ -301,8 +319,18 @@ export default async function routingHandler( | |
| * @param eventOrResult | ||
| * @returns Whether the event is an instance of `InternalResult` | ||
| */ | ||
| function isInternalResult( | ||
| eventOrResult: InternalEvent | InternalResult, | ||
| export function isInternalResult( | ||
| eventOrResult: InternalEvent | InternalResult | PartialResult, | ||
| ): eventOrResult is InternalResult { | ||
| return eventOrResult != null && "statusCode" in eventOrResult; | ||
| } | ||
|
|
||
| /** | ||
| * @param eventOrResult | ||
| * @returns Whether the event is an instance of `PartialResult` (i.e. for PPR responses) | ||
| */ | ||
| export function isPartialResult( | ||
| eventOrResult: InternalEvent | InternalResult | PartialResult, | ||
| ): eventOrResult is PartialResult { | ||
| return eventOrResult != null && "resumeRequest" in eventOrResult; | ||
| } | ||
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.
It would be nice to make it clear that it "Only works with the Adapter API" ?
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.
however I am not sure to understand from the code why/where the logic is scoped to the adapter handler, could you expand on that?
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.
It's on the Next side, basically the resume endpoint is only available using the adapter (or in minimal mode or with some patch)