From 12982efaa2a6249510c0a9e7887934764d56f867 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Tue, 16 Apr 2024 18:01:46 -0700 Subject: [PATCH] fix incorrect refresh request when basePath is set (#64589) `initialCanonicalUrl` differs from the `canonicalUrl` that gets set on the client, such as when there's a basePath set. This hoists the `canonicalUrl` construction up so we can re-use it when adding refetch markers to the tree. This also renames `pathname` -> `path` since it also includes search params. I added a test to confirm no extra erroneous fetches happened in both cases. Fixes #64584 Closes NEXT-3130 --- .../apply-router-state-patch-to-tree.ts | 8 +++--- .../create-initial-router-state.ts | 18 +++++++------ .../refetch-inactive-parallel-segments.ts | 18 ++++++------- .../app-dir/app-basepath/app/refresh/page.js | 16 +++++++++++ test/e2e/app-dir/app-basepath/index.test.ts | 27 +++++++++++++++++++ 5 files changed, 66 insertions(+), 21 deletions(-) create mode 100644 test/e2e/app-dir/app-basepath/app/refresh/page.js diff --git a/packages/next/src/client/components/router-reducer/apply-router-state-patch-to-tree.ts b/packages/next/src/client/components/router-reducer/apply-router-state-patch-to-tree.ts index 7b51e3660215c..c9cd5a7be7a52 100644 --- a/packages/next/src/client/components/router-reducer/apply-router-state-patch-to-tree.ts +++ b/packages/next/src/client/components/router-reducer/apply-router-state-patch-to-tree.ts @@ -80,7 +80,7 @@ export function applyRouterStatePatchToTree( flightSegmentPath: FlightSegmentPath, flightRouterState: FlightRouterState, treePatch: FlightRouterState, - pathname: string + path: string ): FlightRouterState | null { const [segment, parallelRoutes, url, refetch, isRootLayout] = flightRouterState @@ -93,7 +93,7 @@ export function applyRouterStatePatchToTree( flightSegmentPath ) - addRefreshMarkerToActiveParallelSegments(tree, pathname) + addRefreshMarkerToActiveParallelSegments(tree, path) return tree } @@ -119,7 +119,7 @@ export function applyRouterStatePatchToTree( flightSegmentPath.slice(2), parallelRoutes[parallelRouteKey], treePatch, - pathname + path ) if (parallelRoutePatch === null) { @@ -142,7 +142,7 @@ export function applyRouterStatePatchToTree( tree[4] = true } - addRefreshMarkerToActiveParallelSegments(tree, pathname) + addRefreshMarkerToActiveParallelSegments(tree, path) return tree } 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 d4c0e138ad39c..9a8b5bc901dc6 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 @@ -49,7 +49,15 @@ export function createInitialRouterState({ loading: initialSeedData[3], } - addRefreshMarkerToActiveParallelSegments(initialTree, initialCanonicalUrl) + const canonicalUrl = + // location.href is read as the initial value for canonicalUrl in the browser + // This is safe to do as canonicalUrl can't be rendered, it's only used to control the history updates in the useEffect further down in this file. + location + ? // window.location does not have the same type as URL but has all the fields createHrefFromUrl needs. + createHrefFromUrl(location) + : initialCanonicalUrl + + addRefreshMarkerToActiveParallelSegments(initialTree, canonicalUrl) const prefetchCache = new Map() @@ -82,13 +90,7 @@ export function createInitialRouterState({ hashFragment: null, segmentPaths: [], }, - canonicalUrl: - // location.href is read as the initial value for canonicalUrl in the browser - // This is safe to do as canonicalUrl can't be rendered, it's only used to control the history updates in the useEffect further down in this file. - location - ? // window.location does not have the same type as URL but has all the fields createHrefFromUrl needs. - createHrefFromUrl(location) - : initialCanonicalUrl, + canonicalUrl, nextUrl: // the || operator is intentional, the pathname can be an empty string (extractPathFromFlightRouterState(initialTree) || location?.pathname) ?? diff --git a/packages/next/src/client/components/router-reducer/refetch-inactive-parallel-segments.ts b/packages/next/src/client/components/router-reducer/refetch-inactive-parallel-segments.ts index 95eb0ba806408..05c1ddee9c3a2 100644 --- a/packages/next/src/client/components/router-reducer/refetch-inactive-parallel-segments.ts +++ b/packages/next/src/client/components/router-reducer/refetch-inactive-parallel-segments.ts @@ -45,23 +45,23 @@ async function refreshInactiveParallelSegmentsImpl({ fetchedSegments: Set rootTree: FlightRouterState }) { - const [, parallelRoutes, refetchPathname, refetchMarker] = updatedTree + const [, parallelRoutes, refetchPath, refetchMarker] = updatedTree const fetchPromises = [] if ( - refetchPathname && - refetchPathname !== location.pathname && + refetchPath && + refetchPath !== location.pathname + location.search && refetchMarker === 'refresh' && // it's possible for the tree to contain multiple segments that contain data at the same URL // we keep track of them so we can dedupe the requests - !fetchedSegments.has(refetchPathname) + !fetchedSegments.has(refetchPath) ) { - fetchedSegments.add(refetchPathname) // Mark this URL as fetched + fetchedSegments.add(refetchPath) // Mark this URL as fetched // Eagerly kick off the fetch for the refetch path & the parallel routes. This should be fine to do as they each operate // independently on their own cache nodes, and `applyFlightData` will copy anything it doesn't care about from the existing cache. const fetchPromise = fetchServerResponse( - new URL(refetchPathname, location.origin), + new URL(refetchPath, location.origin), // refetch from the root of the updated tree, otherwise it will be scoped to the current segment // and might not contain the data we need to patch in interception route data (such as dynamic params from a previous segment) [rootTree[0], rootTree[1], rootTree[2], 'refetch'], @@ -110,16 +110,16 @@ async function refreshInactiveParallelSegmentsImpl({ */ export function addRefreshMarkerToActiveParallelSegments( tree: FlightRouterState, - pathname: string + path: string ) { const [segment, parallelRoutes, , refetchMarker] = tree // a page segment might also contain concatenated search params, so we do a partial match on the key if (segment.includes(PAGE_SEGMENT_KEY) && refetchMarker !== 'refresh') { - tree[2] = pathname + tree[2] = path tree[3] = 'refresh' } for (const key in parallelRoutes) { - addRefreshMarkerToActiveParallelSegments(parallelRoutes[key], pathname) + addRefreshMarkerToActiveParallelSegments(parallelRoutes[key], path) } } diff --git a/test/e2e/app-dir/app-basepath/app/refresh/page.js b/test/e2e/app-dir/app-basepath/app/refresh/page.js new file mode 100644 index 0000000000000..8c06211ab2559 --- /dev/null +++ b/test/e2e/app-dir/app-basepath/app/refresh/page.js @@ -0,0 +1,16 @@ +'use client' + +import { useRouter } from 'next/navigation' + +export default function Page() { + const router = useRouter() + return ( + + ) +} diff --git a/test/e2e/app-dir/app-basepath/index.test.ts b/test/e2e/app-dir/app-basepath/index.test.ts index 129292542b420..2cb0c1af41745 100644 --- a/test/e2e/app-dir/app-basepath/index.test.ts +++ b/test/e2e/app-dir/app-basepath/index.test.ts @@ -64,5 +64,32 @@ createNextDescribe( expect(await browser.url()).toBe(`${next.url}/base/dynamic/dest`) }) }) + + it.each(['/base/refresh', '/base/refresh?foo=bar'])( + `should only make a single RSC call to the current page (%s)`, + async (path) => { + let rscRequests = [] + const browser = await next.browser(path, { + beforePageLoad(page) { + page.on('request', (request) => { + return request.allHeaders().then((headers) => { + if ( + headers['RSC'.toLowerCase()] === '1' && + // Prefetches also include `RSC` + headers['Next-Router-Prefetch'.toLowerCase()] !== '1' + ) { + rscRequests.push(request.url()) + } + }) + }) + }, + }) + await browser.elementByCss('button').click() + await retry(async () => { + expect(rscRequests.length).toBe(1) + expect(rscRequests[0]).toContain(`${next.url}${path}`) + }) + } + ) } )