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 30, 2024
1 parent 8fbf043 commit 916576c
Show file tree
Hide file tree
Showing 24 changed files with 325 additions and 133 deletions.
50 changes: 1 addition & 49 deletions packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { defaultConfig } from '../server/config-shared'
import devalue from 'next/dist/compiled/devalue'
import findUp from 'next/dist/compiled/find-up'
import { nanoid } from 'next/dist/compiled/nanoid/index.cjs'
import { pathToRegexp } from 'next/dist/compiled/path-to-regexp'
import path from 'path'
import {
STATIC_STATUS_PAGE_GET_INITIAL_PROPS_ERROR,
Expand All @@ -41,9 +40,7 @@ import type {
Redirect,
Rewrite,
RouteHas,
RouteType,
} from '../lib/load-custom-routes'
import { getRedirectStatus, modifyRouteRegex } from '../lib/redirect-status'
import { nonNullable } from '../lib/non-nullable'
import { recursiveDelete } from '../lib/recursive-delete'
import { verifyPartytownSetup } from '../lib/verify-partytown-setup'
Expand Down Expand Up @@ -162,6 +159,7 @@ import { hasCustomExportOutput } from '../export/utils'
import { interopDefault } from '../lib/interop-default'
import { formatDynamicImportPath } from '../lib/format-dynamic-import-path'
import { isInterceptionRouteAppPath } from '../server/future/helpers/interception-routes'
import { buildCustomRoute } from '../lib/build-custom-route'

interface ExperimentalBypassForInfo {
experimentalBypassFor?: RouteHas[]
Expand Down Expand Up @@ -267,52 +265,6 @@ export type RoutesManifest = {
caseSensitive?: boolean
}

export function buildCustomRoute(
type: 'header',
route: Header
): ManifestHeaderRoute
export function buildCustomRoute(
type: 'rewrite',
route: Rewrite
): ManifestRewriteRoute
export function buildCustomRoute(
type: 'redirect',
route: Redirect,
restrictedRedirectPaths: string[]
): ManifestRedirectRoute
export function buildCustomRoute(
type: RouteType,
route: Redirect | Rewrite | Header,
restrictedRedirectPaths?: string[]
): ManifestHeaderRoute | ManifestRewriteRoute | ManifestRedirectRoute {
const compiled = pathToRegexp(route.source, [], {
strict: true,
sensitive: false,
delimiter: '/', // default is `/#?`, but Next does not pass query info
})

let source = compiled.source
if (!route.internal) {
source = modifyRouteRegex(
source,
type === 'redirect' ? restrictedRedirectPaths : undefined
)
}

const regex = normalizeRouteRegex(source)

if (type !== 'redirect') {
return { ...route, regex }
}

return {
...route,
statusCode: getRedirectStatus(route as Redirect),
permanent: undefined,
regex,
}
}

function pageToRoute(page: string) {
const routeRegex = getNamedRouteRegex(page, true)
return {
Expand Down
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
@@ -1,5 +1,3 @@
import { addPathPrefix } from '../../../../shared/lib/router/utils/add-path-prefix'
import { pathHasPrefix } from '../../../../shared/lib/router/utils/path-has-prefix'
import { createHrefFromUrl } from '../create-href-from-url'

/**
Expand All @@ -9,20 +7,19 @@ 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
false
)

// delimit the prefix so we don't conflict with other pages
const nextUrlPrefix = `${nextUrl}%`

// 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 && !pathHasPrefix(pathnameFromUrl, nextUrl)) {
return addPathPrefix(pathnameFromUrl, nextUrlPrefix)
if (nextUrl) {
return `${nextUrl}%${pathnameFromUrl}`
}

return pathnameFromUrl
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
getPrefetchEntryCacheStatus,
} from '../get-prefetch-cache-entry-status'
import { prunePrefetchCache } from './prune-prefetch-cache'
import { prefetchQueue } from './prefetch-reducer'
import { createPrefetchEntry, prefetchQueue } from './prefetch-reducer'
import { createEmptyCacheNode } from '../../app-router'
import { DEFAULT_SEGMENT_KEY } from '../../../../shared/lib/segment'
import {
Expand Down Expand Up @@ -127,45 +127,37 @@ 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) {
const data = fetchServerResponse(
prefetchValues = createPrefetchEntry({
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,
})

state.prefetchCache.set(prefetchCacheKey, newPrefetchValue)
prefetchValues = newPrefetchValue
state.prefetchCache.set(prefetchCacheKey, 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) {
Expand Down Expand Up @@ -319,45 +311,37 @@ 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) {
const data = fetchServerResponse(
prefetchValues = createPrefetchEntry({
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,
})

state.prefetchCache.set(prefetchCacheKey, newPrefetchValue)
prefetchValues = newPrefetchValue
state.prefetchCache.set(prefetchCacheKey, 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type {
PrefetchAction,
ReducerState,
ReadonlyReducerState,
PrefetchCacheEntry,
} from '../router-reducer-types'
import { PrefetchKind } from '../router-reducer-types'
import { prunePrefetchCache } from './prune-prefetch-cache'
Expand All @@ -22,7 +23,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 @@ -51,27 +52,60 @@ export function prefetchReducer(
}
}

// fetchServerResponse is intentionally not awaited so that it can be unwrapped in the navigate-reducer
const serverResponse = prefetchQueue.enqueue(() =>
const newEntry = createPrefetchEntry({
state,
url,
kind: action.kind,
prefetchCacheKey,
})

state.prefetchCache.set(prefetchCacheKey, newEntry)

return state
}

export function createPrefetchEntry({
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,
// 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
)
)
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)
}

// 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
return prefetchResponse
})

const data = prefetchQueue.enqueue(getPrefetchData)

return {
treeAtTimeOfPrefetch: state.tree,
data: serverResponse,
kind: action.kind,
data,
kind,
prefetchTime: Date.now(),
lastUsedTime: null,
})

return state
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ export type FocusAndScrollRef = {

export type PrefetchCacheEntry = {
treeAtTimeOfPrefetch: FlightRouterState
data: Promise<FetchServerResponseResult> | null
data: Promise<FetchServerResponseResult>
kind: PrefetchKind
prefetchTime: number
lastUsedTime: number | null
Expand Down
Loading

0 comments on commit 916576c

Please sign in to comment.