Skip to content

Commit

Permalink
fix incorrect refresh request when basePath is set (#64589)
Browse files Browse the repository at this point in the history
`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
  • Loading branch information
ztanner committed Apr 17, 2024
1 parent 6f27f94 commit 12982ef
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -93,7 +93,7 @@ export function applyRouterStatePatchToTree(
flightSegmentPath
)

addRefreshMarkerToActiveParallelSegments(tree, pathname)
addRefreshMarkerToActiveParallelSegments(tree, path)

return tree
}
Expand All @@ -119,7 +119,7 @@ export function applyRouterStatePatchToTree(
flightSegmentPath.slice(2),
parallelRoutes[parallelRouteKey],
treePatch,
pathname
path
)

if (parallelRoutePatch === null) {
Expand All @@ -142,7 +142,7 @@ export function applyRouterStatePatchToTree(
tree[4] = true
}

addRefreshMarkerToActiveParallelSegments(tree, pathname)
addRefreshMarkerToActiveParallelSegments(tree, path)

return tree
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, PrefetchCacheEntry>()

Expand Down Expand Up @@ -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) ??
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,23 @@ async function refreshInactiveParallelSegmentsImpl({
fetchedSegments: Set<string>
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'],
Expand Down Expand Up @@ -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)
}
}
16 changes: 16 additions & 0 deletions test/e2e/app-dir/app-basepath/app/refresh/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use client'

import { useRouter } from 'next/navigation'

export default function Page() {
const router = useRouter()
return (
<button
onClick={() => {
router.refresh()
}}
>
Refresh
</button>
)
}
27 changes: 27 additions & 0 deletions test/e2e/app-dir/app-basepath/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`)
})
}
)
}
)

0 comments on commit 12982ef

Please sign in to comment.