From 5307eb0c8e5c107944cfddd0597b7ac3b8f3f2b5 Mon Sep 17 00:00:00 2001 From: Zack Tanner Date: Fri, 26 Jan 2024 13:47:19 -0800 Subject: [PATCH 1/3] fix erroneous prefetch cache misses --- .../router-reducer/fetch-server-response.ts | 10 +- .../reducers/navigate-reducer.test.tsx | 6 + .../reducers/navigate-reducer.ts | 72 +++------ .../reducers/prefetch-cache-utils.ts | 151 ++++++++++++++++++ .../reducers/prefetch-reducer.test.tsx | 2 + .../reducers/prefetch-reducer.ts | 36 ++--- .../reducers/server-patch-reducer.test.tsx | 1 + .../router-reducer/router-reducer-types.ts | 3 +- packages/next/src/server/base-server.ts | 22 +++ test/e2e/app-dir/app-prefetch/app/layout.js | 1 + .../(dashboard)/loading.tsx | 3 + .../(dashboard)/page.tsx | 13 ++ .../prefetch-auto-route-groups/fetch-data.ts | 9 ++ .../app/prefetch-auto-route-groups/layout.tsx | 30 ++++ .../sub/bar/page.tsx | 7 + .../sub/foo/page.tsx | 6 + .../app-dir/app-prefetch/prefetching.test.ts | 37 +++++ 17 files changed, 330 insertions(+), 79 deletions(-) create mode 100644 packages/next/src/client/components/router-reducer/reducers/prefetch-cache-utils.ts create mode 100644 test/e2e/app-dir/app-prefetch/app/prefetch-auto-route-groups/(dashboard)/loading.tsx create mode 100644 test/e2e/app-dir/app-prefetch/app/prefetch-auto-route-groups/(dashboard)/page.tsx create mode 100644 test/e2e/app-dir/app-prefetch/app/prefetch-auto-route-groups/fetch-data.ts create mode 100644 test/e2e/app-dir/app-prefetch/app/prefetch-auto-route-groups/layout.tsx create mode 100644 test/e2e/app-dir/app-prefetch/app/prefetch-auto-route-groups/sub/bar/page.tsx create mode 100644 test/e2e/app-dir/app-prefetch/app/prefetch-auto-route-groups/sub/foo/page.tsx diff --git a/packages/next/src/client/components/router-reducer/fetch-server-response.ts b/packages/next/src/client/components/router-reducer/fetch-server-response.ts index b6cb4d43bb4fb..2af83aa8a203c 100644 --- a/packages/next/src/client/components/router-reducer/fetch-server-response.ts +++ b/packages/next/src/client/components/router-reducer/fetch-server-response.ts @@ -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] } /** @@ -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') { @@ -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.`, @@ -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] } } diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx index c8746d2a34fa9..b7cf15ce625f4 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx @@ -256,6 +256,7 @@ describe('navigateReducer', () => { "prefetchCache": Map { "/linking/about" => { "data": Promise {}, + "key": "/linking/about", "kind": "temporary", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, @@ -450,6 +451,7 @@ describe('navigateReducer', () => { "prefetchCache": Map { "/linking/about" => { "data": Promise {}, + "key": "/linking/about", "kind": "temporary", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, @@ -887,6 +889,7 @@ describe('navigateReducer', () => { "prefetchCache": Map { "/linking" => { "data": Promise {}, + "key": "/linking", "kind": "temporary", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, @@ -1113,6 +1116,7 @@ describe('navigateReducer', () => { "prefetchCache": Map { "/linking/about" => { "data": Promise {}, + "key": "/linking/about", "kind": "auto", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, @@ -1367,6 +1371,7 @@ describe('navigateReducer', () => { "prefetchCache": Map { "/parallel-tab-bar/demographics" => { "data": Promise {}, + "key": "/parallel-tab-bar/demographics", "kind": "temporary", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, @@ -1710,6 +1715,7 @@ describe('navigateReducer', () => { "prefetchCache": Map { "/linking/about" => { "data": Promise {}, + "key": "/linking/about", "kind": "temporary", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts index 8c149d0d21230..001fa3aad2ece 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -29,9 +29,11 @@ import { } from '../ppr-navigations' import { createPrefetchCacheKey, - prunePrefetchCache, + getPrefetchCacheEntry, getPrefetchEntryCacheStatus, -} from '../prefetch-cache-utils' + prunePrefetchCache, + createPrefetchCacheEntry, +} from './prefetch-cache-utils' export function handleExternalUrl( state: ReadonlyReducerState, @@ -126,45 +128,30 @@ function navigateReducer_noPPR( return handleExternalUrl(state, mutable, url.toString(), pendingPush) } - const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl) - let prefetchValues = state.prefetchCache.get(prefetchCacheKey) - + let prefetchValues = getPrefetchCacheEntry(url, state) // If we don't have a prefetch value, we need to create one if (!prefetchValues) { - const data = fetchServerResponse( + const cacheKey = createPrefetchCacheKey(url) + prefetchValues = createPrefetchCacheEntry({ + state, url, - state.tree, - state.nextUrl, - state.buildId, // in dev, there's never gonna be a prefetch entry so we want to prefetch here - // in order to simulate the behavior of the prefetch cache - process.env.NODE_ENV === 'development' ? PrefetchKind.AUTO : undefined - ) - - const newPrefetchValue = { - data, - // this will make sure that the entry will be discarded after 30s kind: process.env.NODE_ENV === 'development' ? PrefetchKind.AUTO : PrefetchKind.TEMPORARY, - prefetchTime: Date.now(), - treeAtTimeOfPrefetch: state.tree, - lastUsedTime: null, - } + prefetchCacheKey: cacheKey, + }) - state.prefetchCache.set(prefetchCacheKey, newPrefetchValue) - prefetchValues = newPrefetchValue + state.prefetchCache.set(cacheKey, prefetchValues) } const prefetchEntryCacheStatus = getPrefetchEntryCacheStatus(prefetchValues) - // The one before last item is the router state tree patch const { treeAtTimeOfPrefetch, data } = prefetchValues + prefetchQueue.bump(data) - prefetchQueue.bump(data!) - - return data!.then( + return data.then( ([flightData, canonicalUrlOverride]) => { // we only want to mark this once if (prefetchValues && !prefetchValues.lastUsedTime) { @@ -318,45 +305,30 @@ function navigateReducer_PPR( return handleExternalUrl(state, mutable, url.toString(), pendingPush) } - const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl) - let prefetchValues = state.prefetchCache.get(prefetchCacheKey) - + let prefetchValues = getPrefetchCacheEntry(url, state) // If we don't have a prefetch value, we need to create one if (!prefetchValues) { - const data = fetchServerResponse( + const cacheKey = createPrefetchCacheKey(url) + prefetchValues = createPrefetchCacheEntry({ + state, url, - state.tree, - state.nextUrl, - state.buildId, // in dev, there's never gonna be a prefetch entry so we want to prefetch here - // in order to simulate the behavior of the prefetch cache - process.env.NODE_ENV === 'development' ? PrefetchKind.AUTO : undefined - ) - - const newPrefetchValue = { - data, - // this will make sure that the entry will be discarded after 30s kind: process.env.NODE_ENV === 'development' ? PrefetchKind.AUTO : PrefetchKind.TEMPORARY, - prefetchTime: Date.now(), - treeAtTimeOfPrefetch: state.tree, - lastUsedTime: null, - } + prefetchCacheKey: cacheKey, + }) - state.prefetchCache.set(prefetchCacheKey, newPrefetchValue) - prefetchValues = newPrefetchValue + state.prefetchCache.set(cacheKey, prefetchValues) } const prefetchEntryCacheStatus = getPrefetchEntryCacheStatus(prefetchValues) - // The one before last item is the router state tree patch const { treeAtTimeOfPrefetch, data } = prefetchValues + prefetchQueue.bump(data) - prefetchQueue.bump(data!) - - return data!.then( + return data.then( ([flightData, canonicalUrlOverride, _postponed]) => { // we only want to mark this once if (prefetchValues && !prefetchValues.lastUsedTime) { diff --git a/packages/next/src/client/components/router-reducer/reducers/prefetch-cache-utils.ts b/packages/next/src/client/components/router-reducer/reducers/prefetch-cache-utils.ts new file mode 100644 index 0000000000000..e96432f4a5342 --- /dev/null +++ b/packages/next/src/client/components/router-reducer/reducers/prefetch-cache-utils.ts @@ -0,0 +1,151 @@ +import { createHrefFromUrl } from '../create-href-from-url' +import { fetchServerResponse } from '../fetch-server-response' +import type { + PrefetchCacheEntry, + PrefetchKind, + ReadonlyReducerState, +} from '../router-reducer-types' +import { prefetchQueue } from './prefetch-reducer' + +/** + * Creates a cache key for the router prefetch cache + * + * @param url - The URL being navigated to + * @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) { + const pathnameFromUrl = createHrefFromUrl( + url, + // Ensures the hash is not part of the cache key as it does not impact the server fetch + false + ) + + // delimit the prefix so we don't conflict with other pages + + // Route interception depends on `nextUrl` values which aren't a 1:1 mapping to a URL + // The cache key that we store needs to use `nextUrl` to properly distinguish cache entries + if (nextUrl) { + return `${nextUrl}%${pathnameFromUrl}` + } + + return pathnameFromUrl +} + +export function getPrefetchCacheEntry( + url: URL, + state: ReadonlyReducerState +): PrefetchCacheEntry | undefined { + // We first check if there's a more specific interception route prefetch entry + // This is because when we detect a prefetch that corresponds with an interception route, we prefix it with nextUrl (see `createPrefetchCacheKey`) + // to avoid conflicts with other pages that may have the same URL but render different things depending on the `Next-URL` header. + const interceptionCacheKey = createPrefetchCacheKey(url, state.nextUrl) + const interceptionData = state.prefetchCache.get(interceptionCacheKey) + + if (interceptionData) { + return interceptionData + } + + // If we dont find a more specific interception route prefetch entry, we check for a regular prefetch entry + const prefetchCacheKey = createPrefetchCacheKey(url) + return state.prefetchCache.get(prefetchCacheKey) +} + +export function createPrefetchCacheEntry({ + state, + url, + kind, + prefetchCacheKey, +}: { + state: ReadonlyReducerState + url: URL + kind: PrefetchKind + prefetchCacheKey: string +}): PrefetchCacheEntry { + // initiates the fetch request for the prefetch and attaches a listener + // to the promise to update the prefetch cache entry when the promise resolves (if necessary) + const getPrefetchData = () => + fetchServerResponse( + url, + state.tree, + state.nextUrl, + state.buildId, + kind + ).then((prefetchResponse) => { + /* [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 + }) + + const data = prefetchQueue.enqueue(getPrefetchData) + + return { + treeAtTimeOfPrefetch: state.tree, + data, + kind, + prefetchTime: Date.now(), + lastUsedTime: null, + key: prefetchCacheKey, + } +} + +export function prunePrefetchCache( + prefetchCache: ReadonlyReducerState['prefetchCache'] +) { + for (const [href, prefetchCacheEntry] of prefetchCache) { + if ( + getPrefetchEntryCacheStatus(prefetchCacheEntry) === + PrefetchCacheEntryStatus.expired + ) { + prefetchCache.delete(href) + } + } +} + +const FIVE_MINUTES = 5 * 60 * 1000 +const THIRTY_SECONDS = 30 * 1000 + +export enum PrefetchCacheEntryStatus { + fresh = 'fresh', + reusable = 'reusable', + expired = 'expired', + stale = 'stale', +} + +export function getPrefetchEntryCacheStatus({ + kind, + prefetchTime, + lastUsedTime, +}: PrefetchCacheEntry): PrefetchCacheEntryStatus { + // if the cache entry was prefetched or read less than 30s ago, then we want to re-use it + if (Date.now() < (lastUsedTime ?? prefetchTime) + THIRTY_SECONDS) { + return lastUsedTime + ? PrefetchCacheEntryStatus.reusable + : PrefetchCacheEntryStatus.fresh + } + + // if the cache entry was prefetched less than 5 mins ago, then we want to re-use only the loading state + if (kind === 'auto') { + if (Date.now() < prefetchTime + FIVE_MINUTES) { + return PrefetchCacheEntryStatus.stale + } + } + + // if the cache entry was prefetched less than 5 mins ago and was a "full" prefetch, then we want to re-use it "full + if (kind === 'full') { + if (Date.now() < prefetchTime + FIVE_MINUTES) { + return PrefetchCacheEntryStatus.reusable + } + } + + return PrefetchCacheEntryStatus.expired +} diff --git a/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.test.tsx b/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.test.tsx index d77e7218b6384..88efb764f6b3a 100644 --- a/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.test.tsx +++ b/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.test.tsx @@ -148,6 +148,7 @@ describe('prefetchReducer', () => { [ '/linking/about', { + key: '/linking/about', data: prom, kind: PrefetchKind.AUTO, lastUsedTime: null, @@ -304,6 +305,7 @@ describe('prefetchReducer', () => { [ '/linking/about', { + key: '/linking/about', data: prom, prefetchTime: expect.any(Number), kind: PrefetchKind.AUTO, diff --git a/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.ts index 306550be96bac..851a648538303 100644 --- a/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.ts @@ -1,4 +1,3 @@ -import { fetchServerResponse } from '../fetch-server-response' import type { PrefetchAction, ReducerState, @@ -9,8 +8,10 @@ import { NEXT_RSC_UNION_QUERY } from '../../app-router-headers' import { PromiseQueue } from '../../promise-queue' import { createPrefetchCacheKey, + createPrefetchCacheEntry, + getPrefetchCacheEntry, prunePrefetchCache, -} from '../prefetch-cache-utils' +} from './prefetch-cache-utils' export const prefetchQueue = new PromiseQueue(5) @@ -24,8 +25,7 @@ export function prefetchReducer( const { url } = action url.searchParams.delete(NEXT_RSC_UNION_QUERY) - const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl) - const cacheEntry = state.prefetchCache.get(prefetchCacheKey) + const cacheEntry = getPrefetchCacheEntry(url, state) if (cacheEntry) { /** @@ -33,7 +33,7 @@ export function prefetchReducer( * where we didn't have the prefetch intent. We want to update it to the new, more accurate, kind here. */ if (cacheEntry.kind === PrefetchKind.TEMPORARY) { - state.prefetchCache.set(prefetchCacheKey, { + state.prefetchCache.set(cacheEntry.key, { ...cacheEntry, kind: action.kind, }) @@ -53,27 +53,15 @@ export function prefetchReducer( } } - // fetchServerResponse is intentionally not awaited so that it can be unwrapped in the navigate-reducer - const serverResponse = prefetchQueue.enqueue(() => - 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 - ) - ) - - // 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, + const prefetchCacheKey = createPrefetchCacheKey(url) + const newEntry = createPrefetchCacheEntry({ + state, + url, kind: action.kind, - prefetchTime: Date.now(), - lastUsedTime: null, + prefetchCacheKey, }) + state.prefetchCache.set(prefetchCacheKey, newEntry) + return state } diff --git a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx index 47773163997aa..3ad5b14e2b146 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx +++ b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx @@ -444,6 +444,7 @@ describe('serverPatchReducer', () => { "prefetchCache": Map { "/linking/about" => { "data": Promise {}, + "key": "/linking/about", "kind": "temporary", "lastUsedTime": 1690329600000, "prefetchTime": 1690329600000, diff --git a/packages/next/src/client/components/router-reducer/router-reducer-types.ts b/packages/next/src/client/components/router-reducer/router-reducer-types.ts index ab034432ebaf3..bd6c90f11f6d2 100644 --- a/packages/next/src/client/components/router-reducer/router-reducer-types.ts +++ b/packages/next/src/client/components/router-reducer/router-reducer-types.ts @@ -198,10 +198,11 @@ export type FocusAndScrollRef = { export type PrefetchCacheEntry = { treeAtTimeOfPrefetch: FlightRouterState - data: Promise | null + data: Promise kind: PrefetchKind prefetchTime: number lastUsedTime: number | null + key: string } export enum PrefetchCacheEntryStatus { diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index a3d173669ce58..0783c16052aa6 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -2035,6 +2035,28 @@ export default abstract class Server { } if (isAppPath) { + res.setHeader('vary', RSC_VARY_HEADER) + + if (isPrefetchRSCRequest) { + const couldBeRewritten = this.interceptionRouteRewrites?.some( + (rewrite) => { + return new RegExp(rewrite.regex).test(resolvedUrlPathname) + } + ) + + // Interception route responses can vary based on the `Next-URL` header as they're rewritten to different components. + // This means that multiple route interception responses can resolve to the same URL. We use the Vary header to signal this + // behavior to the client so that it can properly cache the response. + // If the request that we're handling is one that could have a different response based on the `Next-URL` header, or if + // we're handling an interception route, then we include `Next-URL` in the Vary header. + if ( + couldBeRewritten || + isInterceptionRouteAppPath(resolvedUrlPathname) + ) { + res.setHeader('vary', `${RSC_VARY_HEADER}, ${NEXT_URL}`) + } + } + if (!this.renderOpts.dev && !isPreviewMode && isSSG && isRSCRequest) { // If this is an RSC request but we aren't in minimal mode, then we mark // that this is a data request so that we can generate the flight data diff --git a/test/e2e/app-dir/app-prefetch/app/layout.js b/test/e2e/app-dir/app-prefetch/app/layout.js index c2349110eb852..d766f46be0679 100644 --- a/test/e2e/app-dir/app-prefetch/app/layout.js +++ b/test/e2e/app-dir/app-prefetch/app/layout.js @@ -2,6 +2,7 @@ export default function Root({ children }) { return ( + Hello World