Skip to content

Commit

Permalink
remove optimistic navigation behavior when prefetch is false (#58413)
Browse files Browse the repository at this point in the history
### What?
When navigating between pages (via `prefetch: false`) within a dynamic
segment, the shared layout is re-rendered. This can cause unexpected
behavior like layout data changing when navigating between child
segments.

### Why?
When prefetch is false, we're currently opting into an "optimistic
navigation" codepath, which will optimistically render layout-routers up
to the point where data is missing, while kicking off data fetches. It
attempts to determine where refetching needs to happen by traversing the
router cache nodes and checking where data is missing. However, it
locates these cache nodes by using "segments" obtained by
[deconstructing the
URL](https://github.com/vercel/next.js/blob/fix/optimistic-bailout/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts#L142),
which won't accurately contain dynamic segment data. For ex, `/en` which
corresponds with `/app/[lang]/page.tsx` will have a cache node key of
`lang|en|d`, not `en`. Similarly, the optimistic tree that gets
constructed will also be incorrect, since it uses the URL segment.

### How?
My initial fix was to match the dynamic segment against the segment
constructed by the URL. But after discussion with @sebmarkbage and the
team, it seems more correct to remove the optimistic case all together
as there's no guarantee that the url will actually match to that
segment.

Fixes #50670

---------

Co-authored-by: Tim Neutkens <tim@timneutkens.nl>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
  • Loading branch information
3 people authored Nov 16, 2023
1 parent 8d4f4fc commit 24b2ff1
Show file tree
Hide file tree
Showing 20 changed files with 156 additions and 442 deletions.
17 changes: 3 additions & 14 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -204,15 +204,14 @@ function useChangeByServerResponse(

function useNavigate(dispatch: React.Dispatch<ReducerActions>): RouterNavigate {
return useCallback(
(href, navigateType, forceOptimisticNavigation, shouldScroll) => {
(href, navigateType, shouldScroll) => {
const url = new URL(addBasePath(href), location.href)

return dispatch({
type: ACTION_NAVIGATE,
url,
isExternalUrl: isExternalURL(url),
locationSearch: location.search,
forceOptimisticNavigation,
shouldScroll: shouldScroll ?? true,
navigateType,
cache: createEmptyCacheNode(),
Expand Down Expand Up @@ -330,22 +329,12 @@ function Router({
},
replace: (href, options = {}) => {
startTransition(() => {
navigate(
href,
'replace',
Boolean(options.forceOptimisticNavigation),
options.scroll ?? true
)
navigate(href, 'replace', options.scroll ?? true)
})
},
push: (href, options = {}) => {
startTransition(() => {
navigate(
href,
'push',
Boolean(options.forceOptimisticNavigation),
options.scroll ?? true
)
navigate(href, 'push', options.scroll ?? true)
})
},
refresh: () => {
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ export function fillCacheWithDataProperty(
newCache: CacheNode,
existingCache: CacheNode,
flightSegmentPath: FlightSegmentPath,
fetchResponse: () => Promise<FetchServerResponseResult>,
bailOnParallelRoutes: boolean = false
): { bailOptimistic: boolean } | undefined {
fetchResponse: () => Promise<FetchServerResponseResult>
): void {
const isLastEntry = flightSegmentPath.length <= 2

const [parallelRouteKey, segment] = flightSegmentPath
Expand All @@ -22,24 +21,14 @@ export function fillCacheWithDataProperty(
const existingChildSegmentMap =
existingCache.parallelRoutes.get(parallelRouteKey)

if (
!existingChildSegmentMap ||
(bailOnParallelRoutes && existingCache.parallelRoutes.size > 1)
) {
// Bailout because the existing cache does not have the path to the leaf node
// or the existing cache has multiple parallel routes
// Will trigger lazy fetch in layout-router because of missing segment
return { bailOptimistic: true }
}

let childSegmentMap = newCache.parallelRoutes.get(parallelRouteKey)

if (!childSegmentMap || childSegmentMap === existingChildSegmentMap) {
childSegmentMap = new Map(existingChildSegmentMap)
newCache.parallelRoutes.set(parallelRouteKey, childSegmentMap)
}

const existingChildCacheNode = existingChildSegmentMap.get(cacheKey)
const existingChildCacheNode = existingChildSegmentMap?.get(cacheKey)
let childCacheNode = childSegmentMap.get(cacheKey)

// In case of last segment start off the fetch at this level and don't copy further down.
Expand Down
Loading

0 comments on commit 24b2ff1

Please sign in to comment.