From 5d60dcbffa2b108070d463c1b5d44be407388b64 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Tue, 30 Jul 2024 14:39:26 -0700 Subject: [PATCH] re-use auto prefetch cache entries for different searchParams --- .../create-initial-router-state.ts | 3 +- .../router-reducer/prefetch-cache-utils.ts | 95 ++++++++++--- .../router-reducer/router-reducer-types.ts | 2 +- .../searchparams-reuse-loading/app/layout.tsx | 10 ++ .../searchparams-reuse-loading/app/page.tsx | 24 ++++ .../app/search-params/loading.tsx | 3 + .../app/search-params/page.tsx | 16 +++ .../app/search/layout.tsx | 13 ++ .../app/search/loading.tsx | 3 + .../app/search/page.tsx | 12 ++ .../app/search/search.tsx | 22 +++ .../searchparams-reuse-loading/next.config.js | 6 + .../searchparams-reuse-loading.test.ts | 132 ++++++++++++++++++ 13 files changed, 316 insertions(+), 25 deletions(-) create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/layout.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/page.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/search-params/loading.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/search-params/page.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/search/layout.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/search/loading.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/search/page.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/search/search.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/next.config.js create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts diff --git a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts index 7765b4a8c0514a..b176f90f0452f7 100644 --- a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts +++ b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts @@ -5,7 +5,7 @@ import { createHrefFromUrl } from './create-href-from-url' import { fillLazyItemsTillLeafWithHead } from './fill-lazy-items-till-leaf-with-head' import { extractPathFromFlightRouterState } from './compute-changed-path' import { createPrefetchCacheEntryForInitialLoad } from './prefetch-cache-utils' -import { PrefetchKind, type PrefetchCacheEntry } from './router-reducer-types' +import type { PrefetchCacheEntry } from './router-reducer-types' import { addRefreshMarkerToActiveParallelSegments } from './refetch-inactive-parallel-segments' export interface InitialRouterStateParameters { @@ -101,7 +101,6 @@ export function createInitialRouterState({ createPrefetchCacheEntryForInitialLoad({ url, - kind: PrefetchKind.AUTO, data: { f: initialFlightData, c: undefined, diff --git a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts index 3c36c85be4488c..0435841244716c 100644 --- a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts +++ b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts @@ -1,4 +1,3 @@ -import { createHrefFromUrl } from './create-href-from-url' import { fetchServerResponse } from './fetch-server-response' import { PrefetchCacheEntryStatus, @@ -16,12 +15,27 @@ import type { FetchServerResponseResult } from '../../../server/app-render/types * @param nextUrl - an internal URL, primarily used for handling rewrites. Defaults to '/'. * @return The generated prefetch cache key. */ -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 - ) +function createPrefetchCacheKey( + url: URL, + prefetchKind: PrefetchKind = PrefetchKind.TEMPORARY, + nextUrl?: string | null +) { + // Initially we only use the pathname as the cache key. We don't want to include + // search params so that multiple URLs with the same search parameter can re-use + // loading states. + let pathnameFromUrl = url.pathname + + // RSC responses can differ based on search params, specifically in the case where we aren't + // returning a partial response (ie with `PrefetchKind.AUTO`). + // In the auto case, since loading.js & layout.js won't have access to search params, + // we can safely re-use that cache entry. But for full prefetches, we should not + // re-use the cache entry as the response may differ. + if (prefetchKind === PrefetchKind.FULL) { + // if we have a full prefetch, we can include the search param in the key, + // as we'll be getting back a full response. The server might have read the search + // params when generating the full response. + pathnameFromUrl += url.search + } // nextUrl is used as a cache key delimiter since entries can vary based on the Next-URL header if (nextUrl) { @@ -50,20 +64,31 @@ export function getOrCreatePrefetchCacheEntry({ kind?: PrefetchKind }): PrefetchCacheEntry { let existingCacheEntry: PrefetchCacheEntry | undefined = undefined + const interceptionCacheKey = createPrefetchCacheKey(url, kind, nextUrl) + const interceptionData = prefetchCache.get(interceptionCacheKey) + const fullDataCacheKey = createPrefetchCacheKey(url, PrefetchKind.FULL) + // 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, nextUrl) - const interceptionData = prefetchCache.get(interceptionCacheKey) - if (interceptionData) { existingCacheEntry = interceptionData + } + // Next we check to see if search params are present in the URL: if they are, we want to see if the existing prefetch entry + // is "full" and if so, we use that entry, because those are keyed by the full URL (including search params). This lets + // us re-use the loading state from an "auto" prefetch (if it exists) even for routes that have different search params. + else if (url.search && prefetchCache.has(fullDataCacheKey)) { + console.log('using', { fullDataCacheKey }) + existingCacheEntry = prefetchCache.get(fullDataCacheKey) } else { - // If we dont find a more specific interception route prefetch entry, we check for a regular prefetch entry - const prefetchCacheKey = createPrefetchCacheKey(url) + // Otherwise, we check for a regular prefetch entry. These will be keyed by the pathname only. + const prefetchCacheKey = createPrefetchCacheKey(url, kind) const prefetchData = prefetchCache.get(prefetchCacheKey) if (prefetchData) { + console.log('using regular', { prefetchCacheKey, kind }) existingCacheEntry = prefetchData + } else { + console.log('not found', { prefetchCacheKey, kind }) } } @@ -130,7 +155,11 @@ function prefixExistingPrefetchCacheEntry({ return } - const newCacheKey = createPrefetchCacheKey(url, nextUrl) + const newCacheKey = createPrefetchCacheKey( + url, + existingCacheEntry.kind, + nextUrl + ) prefetchCache.set(newCacheKey, existingCacheEntry) prefetchCache.delete(existingCacheKey) @@ -145,27 +174,31 @@ export function createPrefetchCacheEntryForInitialLoad({ tree, prefetchCache, url, - kind, data, }: Pick & { url: URL - kind: PrefetchKind data: FetchServerResponseResult }) { + // The initial cache entry technically includes full data, but it isn't explicitly prefetched -- we just seed the + // prefetch cache so that we can skip an extra prefetch request later, since we already have the data. + // We use the `full` kind here with a `null` prefetchTime to signal that this entry has the full RSC data, + // but we should only re-use the `loading` portion for up to the `static` staleTime, because keeping the full data + // would be unexpected without explicitly opting into a full prefetch. + const kind = PrefetchKind.FULL // if the prefetch corresponds with an interception route, we use the nextUrl to prefix the cache key const prefetchCacheKey = data.i - ? createPrefetchCacheKey(url, nextUrl) - : createPrefetchCacheKey(url) + ? createPrefetchCacheKey(url, kind, nextUrl) + : createPrefetchCacheKey(url, kind) const prefetchEntry = { treeAtTimeOfPrefetch: tree, data: Promise.resolve(data), kind, - prefetchTime: Date.now(), + prefetchTime: null, lastUsedTime: Date.now(), key: prefetchCacheKey, status: PrefetchCacheEntryStatus.fresh, - } + } satisfies PrefetchCacheEntry prefetchCache.set(prefetchCacheKey, prefetchEntry) @@ -189,7 +222,7 @@ function createLazyPrefetchEntry({ url: URL kind: PrefetchKind }): PrefetchCacheEntry { - const prefetchCacheKey = createPrefetchCacheKey(url) + const prefetchCacheKey = createPrefetchCacheKey(url, kind) // 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) @@ -272,6 +305,24 @@ function getPrefetchEntryCacheStatus({ prefetchTime, lastUsedTime, }: PrefetchCacheEntry): PrefetchCacheEntryStatus { + // TODO: Need to think of a better way to do this. + // Currently the only case of a valid null prefetchTime is when we seed + // the prefetch cache on initial load. + if (prefetchTime === null) { + // We only want to keep around the `loading` portion of these entries as they weren't + // explicit prefetches, and we don't know if they're static. + if ( + kind === PrefetchKind.FULL && + lastUsedTime && + Date.now() < lastUsedTime + STATIC_STALETIME_MS + ) { + return PrefetchCacheEntryStatus.stale + } + + // This is a typeguard -- any other case of `prefetchTime` being null is invalid. + return PrefetchCacheEntryStatus.expired + } + // We will re-use the cache entry data for up to the `dynamic` staletime window. if (Date.now() < (lastUsedTime ?? prefetchTime) + DYNAMIC_STALETIME_MS) { return lastUsedTime @@ -282,14 +333,14 @@ function getPrefetchEntryCacheStatus({ // For "auto" prefetching, we'll re-use only the loading boundary for up to `static` staletime window. // A stale entry will only re-use the `loading` boundary, not the full data. // This will trigger a "lazy fetch" for the full data. - if (kind === 'auto') { + if (kind === PrefetchKind.AUTO) { if (Date.now() < prefetchTime + STATIC_STALETIME_MS) { return PrefetchCacheEntryStatus.stale } } // for "full" prefetching, we'll re-use the cache entry data for up to `static` staletime window. - if (kind === 'full') { + if (kind === PrefetchKind.FULL) { if (Date.now() < prefetchTime + STATIC_STALETIME_MS) { return PrefetchCacheEntryStatus.reusable } 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 dd9a04624f1a38..5d73041dca45a0 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 @@ -203,7 +203,7 @@ export type PrefetchCacheEntry = { treeAtTimeOfPrefetch: FlightRouterState data: Promise kind: PrefetchKind - prefetchTime: number + prefetchTime: number | null lastUsedTime: number | null key: string status: PrefetchCacheEntryStatus diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/layout.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/layout.tsx new file mode 100644 index 00000000000000..00098bb74ebdc4 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/layout.tsx @@ -0,0 +1,10 @@ +import { ReactNode, Suspense } from 'react' +export default function Root({ children }: { children: ReactNode }) { + return ( + + + {children} + + + ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/page.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/page.tsx new file mode 100644 index 00000000000000..3b812c3c7ad1ee --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/page.tsx @@ -0,0 +1,24 @@ +import Link from 'next/link' + +export default function Page() { + return ( + <> + Go to search +
+ + + + ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/search-params/loading.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/search-params/loading.tsx new file mode 100644 index 00000000000000..1d7bf4a78bfc62 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/search-params/loading.tsx @@ -0,0 +1,3 @@ +export default function Loading() { + return

Loading...

+} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/search-params/page.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/search-params/page.tsx new file mode 100644 index 00000000000000..2de15ac420d9ee --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/search-params/page.tsx @@ -0,0 +1,16 @@ +import Link from 'next/link' + +export default async function Page({ + searchParams, +}: { + searchParams: Record +}) { + // sleep for 500ms + await new Promise((resolve) => setTimeout(resolve, 500)) + return ( + <> +

{JSON.stringify(searchParams)}

+ Back + + ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/search/layout.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/search/layout.tsx new file mode 100644 index 00000000000000..17302534856edb --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/search/layout.tsx @@ -0,0 +1,13 @@ +'use client' + +import { Fragment } from 'react' +import { useSearchParams } from 'next/navigation' + +export default function SearchLayout({ + children, +}: { + children: React.ReactNode +}) { + let searchParams = useSearchParams() + return {children} +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/search/loading.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/search/loading.tsx new file mode 100644 index 00000000000000..c39332b81daa04 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/search/loading.tsx @@ -0,0 +1,3 @@ +export default function Loading() { + return

Loading...

+} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/search/page.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/search/page.tsx new file mode 100644 index 00000000000000..fc52f12ec9e37c --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/search/page.tsx @@ -0,0 +1,12 @@ +import { Search } from './search' + +export default async function Page({ searchParams }: { searchParams: any }) { + await new Promise((resolve) => setTimeout(resolve, 3000)) + + return ( +
+ +

Search Value: {searchParams.q ?? 'None'}

+
+ ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/search/search.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/search/search.tsx new file mode 100644 index 00000000000000..c50fae8db3feb5 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/search/search.tsx @@ -0,0 +1,22 @@ +'use client' + +import { useRouter } from 'next/navigation' + +export function Search() { + let router = useRouter() + + function search(event: React.FormEvent) { + event.preventDefault() + + let input = event.currentTarget.q.value + let params = new URLSearchParams([['q', input]]) + router.push(`/search?${params}`) + } + + return ( +
+ + +
+ ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/next.config.js b/test/e2e/app-dir/searchparams-reuse-loading/next.config.js new file mode 100644 index 00000000000000..807126e4cf0bf5 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/next.config.js @@ -0,0 +1,6 @@ +/** + * @type {import('next').NextConfig} + */ +const nextConfig = {} + +module.exports = nextConfig diff --git a/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts b/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts new file mode 100644 index 00000000000000..3349986d4e8269 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts @@ -0,0 +1,132 @@ +import { nextTestSetup } from 'e2e-utils' +import type { Route, Page } from 'playwright' + +describe('searchparams-reuse-loading', () => { + const { next, isNextDev } = nextTestSetup({ + files: __dirname, + }) + + it('should re-use the prefetched loading state when navigating to a new searchParam value', async () => { + const browser = await next.browser('/search') + await browser.waitForElementByCss('#page-content') + + // trigger a transition by submitting a new search + await browser.elementByCss('input').type('test') + await browser.elementByCss('button').click() + + const loading = await browser.waitForElementByCss('#loading') + expect(await loading.text()).toBe('Loading...') + + const searchValue = await browser.waitForElementByCss('#search-value') + expect(await searchValue.text()).toBe('Search Value: test') + + // One more time! + await browser.elementByCss('input').type('another') + await browser.elementByCss('button').click() + + const newLoading = await browser.waitForElementByCss('#loading') + expect(await newLoading.text()).toBe('Loading...') + + const newSearchValue = await browser.waitForElementByCss('#search-value') + expect(await newSearchValue.text()).toBe('Search Value: another') + }) + + // Dev doesn't perform prefetching, so this test is skipped, as it relies on intercepting + // prefetch network requests. + if (!isNextDev) { + it('should correctly return different RSC data for full prefetches with different searchParam values', async () => { + const rscRequestPromise = new Map< + string, + { resolve: () => Promise } + >() + + let interceptRequests = false + const browser = await next.browser('/', { + beforePageLoad(page: Page) { + page.route('**/search-params*', async (route: Route) => { + if (!interceptRequests) { + return route.continue() + } + + const request = route.request() + const headers = await request.allHeaders() + const url = new URL(request.url()) + const promiseKey = + url.pathname + '?id=' + url.searchParams.get('id') + + if (headers['rsc'] === '1' && !headers['next-router-prefetch']) { + // Create a promise that will be resolved by the later test code + let resolvePromise: () => void + const promise = new Promise((res) => { + resolvePromise = res + }) + + if (rscRequestPromise.has(promiseKey)) { + throw new Error('Duplicate request') + } + + rscRequestPromise.set(promiseKey, { + resolve: async () => { + await route.continue() + // wait a moment to ensure the response is received + await new Promise((res) => setTimeout(res, 500)) + resolvePromise() + }, + }) + + // Await the promise to effectively stall the request + await promise + } else { + await route.continue() + } + }) + }, + }) + + await browser.waitForIdleNetwork() + interceptRequests = true + // The first link we click is "auto" prefetched. + await browser.elementByCss('[href="/search-params?id=1"]').click() + + // We expect to click it and immediately see a loading state + expect(await browser.elementById('loading').text()).toBe('Loading...') + // We only resolve the dynamic request after we've confirmed loading exists, + // to avoid a race where the dynamic request handles the loading state instead. + let dynamicRequest = rscRequestPromise.get('/search-params?id=1') + expect(dynamicRequest).toBeDefined() + + // resolve the promise + await dynamicRequest.resolve() + dynamicRequest = undefined + + // Confirm the params are correct + const params = await browser.waitForElementByCss('#params').text() + expect(params).toBe('{"id":"1"}') + + await browser.elementByCss("[href='/']").click() + + // Do the exact same thing again, for another prefetch auto link, to ensure + // loading works as expected and we get different search params + await browser.elementByCss('[href="/search-params?id=2"]').click() + expect(await browser.elementById('loading').text()).toBe('Loading...') + dynamicRequest = rscRequestPromise.get('/search-params?id=2') + expect(dynamicRequest).toBeDefined() + + // resolve the promise + await dynamicRequest.resolve() + dynamicRequest = undefined + + const params2 = await browser.waitForElementByCss('#params').text() + expect(params2).toBe('{"id":"2"}') + + // Dev mode doesn't perform full prefetches, so this test is conditional + await browser.elementByCss("[href='/']").click() + + await browser.elementByCss('[href="/search-params?id=3"]').click() + expect(rscRequestPromise.has('/search-params?id=3')).toBe(false) + // no need to resolve any dynamic requests, as this is a full prefetch + const params3 = await browser.waitForElementByCss('#params').text() + expect(params3).toBe('{"id":"3"}') + }) + } +})