Skip to content

Commit

Permalink
fix erroneous prefetch cache misses
Browse files Browse the repository at this point in the history
  • Loading branch information
ztanner committed Jan 26, 2024
1 parent 7f73ce6 commit 2ee2f4e
Show file tree
Hide file tree
Showing 18 changed files with 186 additions and 23 deletions.
2 changes: 1 addition & 1 deletion packages/next/src/client/components/app-router-headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const NEXT_ROUTER_PREFETCH_HEADER = 'Next-Router-Prefetch' as const
export const NEXT_URL = 'Next-Url' as const
export const RSC_CONTENT_TYPE_HEADER = 'text/x-component' as const
export const RSC_VARY_HEADER =
`${RSC_HEADER}, ${NEXT_ROUTER_STATE_TREE}, ${NEXT_ROUTER_PREFETCH_HEADER}, ${NEXT_URL}` as const
`${RSC_HEADER}, ${NEXT_ROUTER_STATE_TREE}, ${NEXT_ROUTER_PREFETCH_HEADER}` as const

export const FLIGHT_PARAMETERS = [
[RSC_HEADER],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ import { hexHash } from '../../../shared/lib/hash'
export type FetchServerResponseResult = [
flightData: FlightData,
canonicalUrlOverride: URL | undefined,
postponed?: boolean
postponed?: boolean,
intercepted?: boolean
]

function doMpaNavigation(url: string): FetchServerResponseResult {
return [urlToUrlWithoutFlightMarker(url).toString(), undefined]
return [urlToUrlWithoutFlightMarker(url).toString(), undefined, false, false]
}

/**
Expand Down Expand Up @@ -112,6 +113,7 @@ export async function fetchServerResponse(

const contentType = res.headers.get('content-type') || ''
const postponed = !!res.headers.get(NEXT_DID_POSTPONE_HEADER)
const interception = !!res.headers.get('vary')?.includes(NEXT_URL)
let isFlightResponse = contentType === RSC_CONTENT_TYPE_HEADER

if (process.env.NODE_ENV === 'production') {
Expand Down Expand Up @@ -145,7 +147,7 @@ export async function fetchServerResponse(
return doMpaNavigation(res.url)
}

return [flightData, canonicalUrl, postponed]
return [flightData, canonicalUrl, postponed, interception]
} catch (err) {
console.error(
`Failed to fetch RSC payload for ${url}. Falling back to browser navigation.`,
Expand All @@ -154,6 +156,6 @@ export async function fetchServerResponse(
// If fetch fails handle it like a mpa navigation
// TODO-APP: Add a test for the case where a CORS request fails, e.g. external url redirect coming from the response.
// See https://github.com/vercel/next.js/issues/43605#issuecomment-1451617521 for a reproduction.
return [url.toString(), undefined]
return [url.toString(), undefined, false, false]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { createHrefFromUrl } from '../create-href-from-url'
* @param nextUrl - an internal URL, primarily used for handling rewrites. Defaults to '/'.
* @return The generated prefetch cache key.
*/
export function createPrefetchCacheKey(url: URL, nextUrl: string | null) {
export function createPrefetchCacheKey(url: URL, nextUrl?: string | null) {
const pathnameFromUrl = createHrefFromUrl(
url,
// Ensures the hash is not part of the cache key as it does not impact the server fetch
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,14 @@ function navigateReducer_noPPR(
return handleExternalUrl(state, mutable, url.toString(), pendingPush)
}

const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchValues = state.prefetchCache.get(prefetchCacheKey)
const prefetchCacheKey = createPrefetchCacheKey(url)
const interceptionCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchValues =
// first check if there's a more specific interception route prefetch entry
// as we don't want to potentially re-use a cache node that would resolve to the same URL
// but renders differently when intercepted
state.prefetchCache.get(interceptionCacheKey) ||
state.prefetchCache.get(prefetchCacheKey)

// If we don't have a prefetch value, we need to create one
if (!prefetchValues) {
Expand Down Expand Up @@ -319,8 +325,14 @@ function navigateReducer_PPR(
return handleExternalUrl(state, mutable, url.toString(), pendingPush)
}

const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchValues = state.prefetchCache.get(prefetchCacheKey)
const prefetchCacheKey = createPrefetchCacheKey(url)
const interceptionCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchValues =
// first check if there's a more specific interception route prefetch entry
// as we don't want to potentially re-use a cache node that would resolve to the same URL
// but renders differently when intercepted
state.prefetchCache.get(interceptionCacheKey) ||
state.prefetchCache.get(prefetchCacheKey)

// If we don't have a prefetch value, we need to create one
if (!prefetchValues) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function prefetchReducer(
const { url } = action
url.searchParams.delete(NEXT_RSC_UNION_QUERY)

const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchCacheKey = createPrefetchCacheKey(url)
const cacheEntry = state.prefetchCache.get(prefetchCacheKey)

if (cacheEntry) {
Expand Down Expand Up @@ -52,20 +52,31 @@ export function prefetchReducer(
}

// fetchServerResponse is intentionally not awaited so that it can be unwrapped in the navigate-reducer
const serverResponse = prefetchQueue.enqueue(() =>
fetchServerResponse(
const serverResponse = prefetchQueue.enqueue(async () => {
const prefetchResponse = await fetchServerResponse(
url,
// initialTree is used when history.state.tree is missing because the history state is set in `useEffect` below, it being missing means this is the hydration case.
state.tree,
state.nextUrl,
state.buildId,
action.kind
)
)

/* [flightData, canonicalUrlOverride, postpone, intercept] */
const [, , , intercept] = prefetchResponse
const existingPrefetchEntry = state.prefetchCache.get(prefetchCacheKey)
// If we discover that the prefetch corresponds with an interception route, we want to move it to
// a prefixed cache key to avoid clobbering an existing entry.
if (intercept && existingPrefetchEntry) {
const prefixedCacheKey = createPrefetchCacheKey(url, state.nextUrl)
state.prefetchCache.set(prefixedCacheKey, existingPrefetchEntry)
state.prefetchCache.delete(prefetchCacheKey)
}

return prefetchResponse
})

// Create new tree based on the flightSegmentPath and router state patch
state.prefetchCache.set(prefetchCacheKey, {
// Create new tree based on the flightSegmentPath and router state patch
treeAtTimeOfPrefetch: state.tree,
data: serverResponse,
kind: action.kind,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
isInterceptionRouteAppPath,
} from '../server/future/helpers/interception-routes'
import type { Rewrite } from './load-custom-routes'
import type { ManifestRewriteRoute } from '../build'

// a function that converts normalised paths (e.g. /foo/[bar]/[baz]) to the format expected by pathToRegexp (e.g. /foo/:bar/:baz)
function toPathToRegexpPath(path: string): string {
Expand Down Expand Up @@ -86,3 +87,8 @@ export function generateInterceptionRoutesRewrites(

return rewrites
}

export function isInterceptionRouteRewrite(route: ManifestRewriteRoute) {
// When we generate interception rewrites in the above implementation, we always do so with only a single `has` condition.
return route.has?.[0].key === NEXT_URL
}
28 changes: 28 additions & 0 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ import {
NEXT_RSC_UNION_QUERY,
NEXT_ROUTER_PREFETCH_HEADER,
NEXT_DID_POSTPONE_HEADER,
NEXT_URL,
} from '../client/components/app-router-headers'
import type {
MatchOptions,
Expand Down Expand Up @@ -129,6 +130,8 @@ import {
import { PrefetchRSCPathnameNormalizer } from './future/normalizers/request/prefetch-rsc'
import { NextDataPathnameNormalizer } from './future/normalizers/request/next-data'
import { getIsServerAction } from './lib/server-action-request-meta'
import { isInterceptionRouteAppPath } from './future/helpers/interception-routes'
import { isInterceptionRouteRewrite } from '../lib/generate-interception-routes-rewrites'

export type FindComponentsResult = {
components: LoadComponentsReturnType
Expand Down Expand Up @@ -2805,6 +2808,31 @@ export default abstract class Server<ServerOptions extends Options = Options> {
res.statusCode = cachedData.status
}

if (isPrefetchRSCRequest && routeModule) {
// when prefetching interception routes, the prefetch cache key can vary based on Next-URL
// as multiple interception routes might resolve to the same URL but map to different components

// interception routes are implemented as beforeFiles rewrites
const beforeFilesRewrites =
this.getRoutesManifest()?.rewrites.beforeFiles

const interceptionRewrites = beforeFilesRewrites?.filter(
isInterceptionRouteRewrite
)

const couldBeRewritten = interceptionRewrites?.some((r) => {
// verify if the path for the current request corresponds with an interception route
return new RegExp(r.regex).test(routeModule.definition.pathname)
})

if (
couldBeRewritten ||
isInterceptionRouteAppPath(routeModule.definition.pathname)
) {
res.setHeader('vary', `${RSC_VARY_HEADER}, ${NEXT_URL}`)
}
}

// Mark that the request did postpone if this is a data request.
if (cachedData.postponed && isRSCRequest) {
res.setHeader(NEXT_DID_POSTPONE_HEADER, '1')
Expand Down
1 change: 1 addition & 0 deletions test/e2e/app-dir/app-prefetch/app/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export default function Root({ children }) {
return (
<html>
<head>
<script src="https://cdn.tailwindcss.com?plugins=typography"></script>
<title>Hello World</title>
<script
dangerouslySetInnerHTML={{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Loading() {
return <h1>Loading...</h1>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { fetchCount, fetchData } from '../fetch-data'

export default async function Home() {
const data = await fetchData()
return (
<h1>
Dashboard {data}
<div>
Fetch Count: <span id="count">{fetchCount}</span>
</div>
</h1>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export let fetchCount = 0
export async function fetchData(): Promise<string> {
return new Promise((resolve, reject) => {
setTimeout(() => {
resolve('data')
fetchCount++
}, 1000)
})
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
export const dynamic = 'force-dynamic'

import Link from 'next/link'

export default function Layout({ children }: { children: React.ReactNode }) {
return (
<>
<nav className="bg-gray-800">
<ul className="flex">
<li className="mr-6">
<Link href="/prefetch-auto-route-groups">
<p className="text-white hover:text-gray-300">Dashboard</p>
</Link>
</li>
<li className="mr-6">
<Link href="/prefetch-auto-route-groups/sub/foo">
<p className="text-white hover:text-gray-300">Foo</p>
</Link>
</li>
<li className="mr-6">
<Link href="/prefetch-auto-route-groups/sub/bar">
<p className="text-white hover:text-gray-300">Bar</p>
</Link>
</li>
</ul>
</nav>
{children}
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { fetchData } from '../../fetch-data'

export default async function Integrations() {
const data = await fetchData()

return <h1>Bar Page {data}</h1>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { fetchData } from '../../fetch-data'

export default async function Home() {
const data = await fetchData()
return <h1>Foo Page {data}</h1>
}
37 changes: 37 additions & 0 deletions test/e2e/app-dir/app-prefetch/prefetching.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,43 @@ createNextDescribe(
)
})
})

it('should not re-fetch cached data when navigating back to a route group', async () => {
const browser = await next.browser('/prefetch-auto-route-groups')
// once the page has loaded, we expect a data fetch
expect(await browser.elementById('count').text()).toBe('1')

// once navigating to a sub-page, we expect another data fetch
await browser
.elementByCss("[href='/prefetch-auto-route-groups/sub/foo']")
.click()

// navigating back to the route group page shouldn't trigger any data fetch
await browser
.elementByCss("[href='/prefetch-auto-route-groups']")
.click()

// confirm that the dashboard page is still rendering the stale fetch count, as it should be cached
expect(await browser.elementById('count').text()).toBe('1')

// navigating to a new sub-page, we expect another data fetch
await browser
.elementByCss("[href='/prefetch-auto-route-groups/sub/bar']")
.click()

// finally, going back to the route group page shouldn't trigger any data fetch
await browser
.elementByCss("[href='/prefetch-auto-route-groups']")
.click()

// confirm that the dashboard page is still rendering the stale fetch count, as it should be cached
expect(await browser.elementById('count').text()).toBe('1')

await browser.refresh()
// reloading the page, we should now get an accurate total number of fetches
// the initial fetch, 2 sub-page fetches, and a final fetch when reloading the page
expect(await browser.elementById('count').text()).toBe('4')
})
})
}
)
6 changes: 3 additions & 3 deletions test/e2e/app-dir/app/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ createNextDescribe(
const res = await next.fetch('/dashboard')
expect(res.headers.get('x-edge-runtime')).toBe('1')
expect(res.headers.get('vary')).toBe(
'RSC, Next-Router-State-Tree, Next-Router-Prefetch, Next-Url'
'RSC, Next-Router-State-Tree, Next-Router-Prefetch'
)
})

Expand All @@ -305,8 +305,8 @@ createNextDescribe(
})
expect(res.headers.get('vary')).toBe(
isNextDeploy
? 'RSC, Next-Router-State-Tree, Next-Router-Prefetch, Next-Url'
: 'RSC, Next-Router-State-Tree, Next-Router-Prefetch, Next-Url, Accept-Encoding'
? 'RSC, Next-Router-State-Tree, Next-Router-Prefetch'
: 'RSC, Next-Router-State-Tree, Next-Router-Prefetch, Accept-Encoding'
)
})

Expand Down
3 changes: 1 addition & 2 deletions test/integration/custom-routes/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2558,8 +2558,7 @@ const runTests = (isDev = false) => {
header: 'RSC',
contentTypeHeader: 'text/x-component',
didPostponeHeader: 'x-nextjs-postponed',
varyHeader:
'RSC, Next-Router-State-Tree, Next-Router-Prefetch, Next-Url',
varyHeader: 'RSC, Next-Router-State-Tree, Next-Router-Prefetch',
prefetchHeader: 'Next-Router-Prefetch',
prefetchSuffix: '.prefetch.rsc',
suffix: '.rsc',
Expand Down
3 changes: 1 addition & 2 deletions test/integration/dynamic-routing/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1493,8 +1493,7 @@ function runTests({ dev }) {
header: 'RSC',
contentTypeHeader: 'text/x-component',
didPostponeHeader: 'x-nextjs-postponed',
varyHeader:
'RSC, Next-Router-State-Tree, Next-Router-Prefetch, Next-Url',
varyHeader: 'RSC, Next-Router-State-Tree, Next-Router-Prefetch',
prefetchHeader: 'Next-Router-Prefetch',
prefetchSuffix: '.prefetch.rsc',
suffix: '.rsc',
Expand Down

0 comments on commit 2ee2f4e

Please sign in to comment.