Skip to content

Commit

Permalink
Fix: Prefetching lazily generated param
Browse files Browse the repository at this point in the history
Fixes an oversight in #72168 where the segment data was not correctly
transferred from the render result to the cache entry in the case where
a prerender is lazily generated after build.

I also took the opportunity to remove one of the intermediate types
used by the various layers that the segment data passes through. For
some reason, in the original PR I made the type of `segmentData` on
CachedAppPageValue an object while the corresponding type on
AppPageRenderResultMetadata was a map. (I didn't really notice before
because in the case where the entry is generated at built time, it gets
written to disk then read back out, so there's some data conversion
happening anyway.)
  • Loading branch information
acdlite committed Dec 10, 2024
1 parent b7c271d commit 5d4df5d
Show file tree
Hide file tree
Showing 9 changed files with 168 additions and 33 deletions.
6 changes: 3 additions & 3 deletions packages/next/src/export/routes/app-page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export async function exportAppPage(
postponed,
fetchTags,
fetchMetrics,
segmentFlightData,
segmentData,
} = metadata

// Ensure we don't postpone without having PPR enabled.
Expand Down Expand Up @@ -200,7 +200,7 @@ export async function exportAppPage(
flightData
)

if (segmentFlightData) {
if (segmentData) {
// Emit the per-segment prefetch data. We emit them as separate files
// so that the cache handler has the option to treat each as a
// separate entry.
Expand All @@ -210,7 +210,7 @@ export async function exportAppPage(
RSC_SEGMENTS_DIR_SUFFIX
)
const tasks = []
for (const [segmentPath, buffer] of segmentFlightData.entries()) {
for (const [segmentPath, buffer] of segmentData) {
segmentPaths.push(segmentPath)
const segmentDataFilePath =
segmentPath === '/'
Expand Down
10 changes: 5 additions & 5 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2844,7 +2844,7 @@ async function prerenderToStream(

const flightData = await streamToBuffer(reactServerResult.asStream())
metadata.flightData = flightData
metadata.segmentFlightData = await collectSegmentData(
metadata.segmentData = await collectSegmentData(
flightData,
finalRenderPrerenderStore,
ComponentMod,
Expand Down Expand Up @@ -3323,7 +3323,7 @@ async function prerenderToStream(
serverPrerenderStreamResult.asStream()
)
metadata.flightData = flightData
metadata.segmentFlightData = await collectSegmentData(
metadata.segmentData = await collectSegmentData(
flightData,
finalClientPrerenderStore,
ComponentMod,
Expand Down Expand Up @@ -3455,7 +3455,7 @@ async function prerenderToStream(

if (shouldGenerateStaticFlightData(workStore)) {
metadata.flightData = flightData
metadata.segmentFlightData = await collectSegmentData(
metadata.segmentData = await collectSegmentData(
flightData,
ssrPrerenderStore,
ComponentMod,
Expand Down Expand Up @@ -3647,7 +3647,7 @@ async function prerenderToStream(
if (shouldGenerateStaticFlightData(workStore)) {
const flightData = await streamToBuffer(reactServerResult.asStream())
metadata.flightData = flightData
metadata.segmentFlightData = await collectSegmentData(
metadata.segmentData = await collectSegmentData(
flightData,
prerenderLegacyStore,
ComponentMod,
Expand Down Expand Up @@ -3801,7 +3801,7 @@ async function prerenderToStream(
reactServerPrerenderResult.asStream()
)
metadata.flightData = flightData
metadata.segmentFlightData = await collectSegmentData(
metadata.segmentData = await collectSegmentData(
flightData,
prerenderLegacyStore,
ComponentMod,
Expand Down
7 changes: 4 additions & 3 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2749,7 +2749,7 @@ export default abstract class Server<
rscData: metadata.flightData,
postponed: metadata.postponed,
status: res.statusCode,
segmentData: undefined,
segmentData: metadata.segmentData,
} satisfies CachedAppPageValue,
revalidate: metadata.revalidate,
isFallback: !!fallbackRouteParams,
Expand Down Expand Up @@ -3057,8 +3057,9 @@ export default abstract class Server<
// it's a 404 — either the segment is fully dynamic, or an invalid segment
// path was requested.
if (cacheEntry.value.segmentData) {
const matchedSegment =
cacheEntry.value.segmentData[segmentPrefetchHeader]
const matchedSegment = cacheEntry.value.segmentData.get(
segmentPrefetchHeader
)
if (matchedSegment !== undefined) {
return {
type: 'rsc',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,14 @@ export default class FileSystemCache implements CacheHandler {
)
} catch {}

let maybeSegmentData: { [segmentPath: string]: string } | undefined
let maybeSegmentData: Map<string, Buffer> | undefined
if (meta?.segmentPaths) {
// Collect all the segment data for this page.
// TODO: To optimize file system reads, we should consider creating
// separate cache entries for each segment, rather than storing them
// all on the page's entry. Though the behavior is
// identical regardless.
const segmentData: { [segmentPath: string]: string } = {}
const segmentData: Map<string, Buffer> = new Map()
maybeSegmentData = segmentData
const segmentsDir = key + RSC_SEGMENTS_DIR_SUFFIX
await Promise.all(
Expand All @@ -213,9 +213,9 @@ export default class FileSystemCache implements CacheHandler {
IncrementalCacheKind.APP_PAGE
)
try {
segmentData[segmentPath] = await this.fs.readFile(
segmentDataFilePath,
'utf8'
segmentData.set(
segmentPath,
await this.fs.readFile(segmentDataFilePath)
)
} catch {
// This shouldn't happen, but if for some reason we fail to
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/render-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export type AppPageRenderResultMetadata = {
fetchTags?: string
fetchMetrics?: FetchMetrics

segmentFlightData?: Map<string, Buffer>
segmentData?: Map<string, Buffer>

/**
* In development, the cache is warmed up before the render. This is attached
Expand Down
4 changes: 2 additions & 2 deletions packages/next/src/server/response-cache/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export interface CachedAppPageValue {
status: number | undefined
postponed: string | undefined
headers: OutgoingHttpHeaders | undefined
segmentData: { [segmentPath: string]: string } | undefined
segmentData: Map<string, Buffer> | undefined
}

export interface CachedPageValue {
Expand Down Expand Up @@ -118,7 +118,7 @@ export interface IncrementalCachedAppPageValue {
headers: OutgoingHttpHeaders | undefined
postponed: string | undefined
status: number | undefined
segmentData: { [segmentPath: string]: string } | undefined
segmentData: Map<string, Buffer> | undefined
}

export interface IncrementalCachedPageValue {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Suspense } from 'react'

async function Content({ params }) {
const { param } = await params
return <div id="target-page-with-lazily-generated-param">Param: {param}</div>
}

export default async function Target({ params }) {
return (
<Suspense fallback="Loading...">
<Content params={params} />
</Suspense>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import Link from 'next/link'

// TODO: Once the appropriate API exists/is implemented, configure the param to
// be statically generated on demand but not at build time (`dynamicParams =
// true` isn't supported when `dynamicIO` is enabled.) For now this test case
// seems to work without extra configuration but it might not in the future.

export default function LazilyGeneratedParamsStartPage() {
return (
<>
<p>
Demonstrates that we can prefetch param that is not generated at build
time but is lazily generated on demand
</p>
<ul>
<li>
<Link href="/lazily-generated-params/some-param-value">Target</Link>
</li>
</ul>
</>
)
}
126 changes: 112 additions & 14 deletions test/e2e/app-dir/segment-cache/basic/segment-cache-basic.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { nextTestSetup } from 'e2e-utils'
import type { Page, Route } from 'playwright'
import type * as Playwright from 'playwright'

describe('segment cache (basic tests)', () => {
const { next, isNextDev, skipped } = nextTestSetup({
Expand All @@ -19,9 +19,9 @@ describe('segment cache (basic tests)', () => {
const prefetchLock = interceptor.lockPrefetches()

const browser = await next.browser('/', {
beforePageLoad(page: Page) {
page.route('**/*', async (route: Route) => {
await interceptor.interceptRoute(route)
beforePageLoad(page: Playwright.Page) {
page.route('**/*', async (route: Playwright.Route) => {
await interceptor.interceptRoute(page, route)
})
},
})
Expand All @@ -43,9 +43,9 @@ describe('segment cache (basic tests)', () => {
it('navigate with prefetched data', async () => {
const interceptor = createRequestInterceptor()
const browser = await next.browser('/', {
beforePageLoad(page: Page) {
page.route('**/*', async (route: Route) => {
await interceptor.interceptRoute(route)
beforePageLoad(page: Playwright.Page) {
page.route('**/*', async (route: Playwright.Route) => {
await interceptor.interceptRoute(page, route)
})
},
})
Expand Down Expand Up @@ -77,12 +77,48 @@ describe('segment cache (basic tests)', () => {
)
})

it('navigate to page with lazily-generated (not at build time) static param', async () => {
const interceptor = createRequestInterceptor()
const browser = await interceptor.waitForPrefetches(async () => {
const b = await next.browser('/lazily-generated-params', {
beforePageLoad(page: Playwright.Page) {
page.route('**/*', async (route: Playwright.Route) => {
await interceptor.interceptRoute(page, route)
})
},
})
await b.elementByCss('a')
return b
})

const navigationsLock = interceptor.lockNavigations()

// Navigate to the test page
const link = await browser.elementByCss('a')
await link.click()

// We should be able to render the page with the dynamic param, because
// it is lazily generated
const target = await browser.elementById(
'target-page-with-lazily-generated-param'
)
expect(await target.innerHTML()).toMatchInlineSnapshot(
`"Param: some-param-value"`
)

await navigationsLock.release()

// TODO: Once #73540 lands we can also test that the dynamic nav was skipped
// const navigations = await navigationsLock.release()
// expect(navigations.size).toBe(0)
})

it('prefetch interception route', async () => {
const interceptor = createRequestInterceptor()
const browser = await next.browser('/interception/feed', {
beforePageLoad(page: Page) {
page.route('**/*', async (route: Route) => {
await interceptor.interceptRoute(route)
beforePageLoad(page: Playwright.Page) {
page.route('**/*', async (route: Playwright.Route) => {
await interceptor.interceptRoute(page, route)
})
},
})
Expand Down Expand Up @@ -112,8 +148,11 @@ function createRequestInterceptor() {
// implementation details as much as possible, so the only thing this does
// for now is let you block and release requests from happening based on
// their type (prefetch requests, navigation requests).
let pendingPrefetches: Set<Route> | null = null
let pendingNavigations: Set<Route> | null = null
let pendingPrefetches: Set<Playwright.Route> | null = null
let pendingNavigations: Set<Playwright.Route> | null = null

let prefetchesPromise: PromiseWithResolvers<void> = null
let lastPrefetchRequest: Playwright.Request | null = null

return {
lockNavigations() {
Expand All @@ -131,6 +170,7 @@ function createRequestInterceptor() {
for (const route of routes) {
route.continue()
}
return routes
},
}
},
Expand All @@ -150,17 +190,75 @@ function createRequestInterceptor() {
for (const route of routes) {
route.continue()
}
return routes
},
}
},

async interceptRoute(route: Route) {
const requestHeaders = await route.request().allHeaders()
/**
* Waits for the next for the next prefetch request, then keeps waiting
* until the prefetch queue is empty (to account for network throttling).
*
* If no prefetches are initiated, this will timeout.
*/
async waitForPrefetches<T>(
scope: () => Promise<T> | T = (): undefined => {}
): Promise<T> {
if (prefetchesPromise === null) {
let resolve
let reject
const promise: Promise<void> = new Promise((res, rej) => {
resolve = res
reject = rej
})
prefetchesPromise = {
resolve,
reject,
promise,
}
}
const result = await scope()
if (prefetchesPromise !== null) {
await prefetchesPromise.promise
}
return result
},

async interceptRoute(page: Playwright.Page, route: Playwright.Route) {
const request = route.request()
const requestHeaders = await request.allHeaders()

if (requestHeaders['RSC'.toLowerCase()]) {
// This is an RSC request. Check if it's a prefetch or a navigation.
if (requestHeaders['Next-Router-Prefetch'.toLowerCase()]) {
// This is a prefetch request.
if (prefetchesPromise !== null) {
// Wait for the prefetch response to finish, then wait an additional
// async task for additional prefetches to be initiated.
lastPrefetchRequest = request
const waitForMorePrefetches = async () => {
const response = await request.response()
await response.finished()
await page.evaluate(
() =>
// If the prefetch queue is network throttled, the next
// request should be issued within a microtask of the previous
// one finishing.
new Promise<void>((res) => requestIdleCallback(() => res()))
)
if (request === lastPrefetchRequest) {
// No further prefetches were initiated. Assume the prefetch
// queue is now empty.
prefetchesPromise.resolve()
prefetchesPromise = null
lastPrefetchRequest = null
}
}
waitForMorePrefetches().then(
() => {},
() => {}
)
}
if (pendingPrefetches !== null) {
pendingPrefetches.add(route)
return
Expand Down

0 comments on commit 5d4df5d

Please sign in to comment.