Skip to content

Commit

Permalink
fix navigation applying stale data when triggered from global not fou…
Browse files Browse the repository at this point in the history
…nd (#62033)

### What
When a global not found page is rendered, and when the not-found page or
containing layout has a link with `prefetch: auto` back to the root
page, the router would update the URL but not correctly swap out the
not-found component with the page component.

### Why
With auto prefetching (which is the default when `prefetch` is left
unspecified on a link), the router will perform a partial prefetch on
dynamic pages. This means it'll fetch the flight data _without_ React
nodes and store it in the prefetch cache. On navigation, this is used to
determine where we already have cached React nodes and where we need to
trigger a lazy fetch to get new data. However, global not found pages
are peculiar in that they will always contain a data path like: `['', {
children: ['__PAGE__', {}] }]` since they are inserted at the root. This
means that if there's also a page component that corresponds with the
same path, the router will incorrectly think it already has cache node
data for it.

### How
During SSR when the `asNotFound` flag signals to the renderer that the
component we're rendering is matching the global not-found page, we
modify the segment key to be something unique so the data path won't
collide with a top-level page.

In
[fc01c8e](fc01c8e)
I added handling only on the server to modify the segment key. This
still fixes the issue, but at the cost of triggering an MPA navigation
on the client because it's treated as a root layout change

In
[69d5687](69d5687)
I added client handling to not treat this special segment key as a root
layout change, and to signal to the router it needs to refetch the data.
This ensures we don't do an MPA navigation.

Fixes #61956
Closes NEXT-2481

---------

Co-authored-by: JJ Kasper <jj@jjsweb.site>
  • Loading branch information
ztanner and ijjk committed Feb 14, 2024
1 parent 56d35ed commit fffa4c3
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { FlightRouterState } from '../../../server/app-render/types'
import { GLOBAL_NOT_FOUND_SEGMENT_KEY } from '../../../shared/lib/segment'

export function isNavigatingToNewRootLayout(
currentTree: FlightRouterState,
Expand All @@ -7,6 +8,10 @@ export function isNavigatingToNewRootLayout(
// Compare segments
const currentTreeSegment = currentTree[0]
const nextTreeSegment = nextTree[0]

// We currently special-case the global not found segment key, but we don't want it to be treated as a root layout change
if (currentTreeSegment === GLOBAL_NOT_FOUND_SEGMENT_KEY) return false

// If any segment is different before we find the root layout, the root layout has changed.
// E.g. /same/(group1)/layout.js -> /same/(group2)/layout.js
// First segment is 'same' for both, keep looking. (group1) changed to (group2) before the root layout was found, it must have changed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ import { handleMutable } from '../handle-mutable'
import { applyFlightData } from '../apply-flight-data'
import { prefetchQueue } from './prefetch-reducer'
import { createEmptyCacheNode } from '../../app-router'
import { DEFAULT_SEGMENT_KEY } from '../../../../shared/lib/segment'
import {
DEFAULT_SEGMENT_KEY,
GLOBAL_NOT_FOUND_SEGMENT_KEY,
} from '../../../../shared/lib/segment'
import {
listenForDynamicRequest,
updateCacheNodeOnNavigation,
Expand Down Expand Up @@ -202,7 +205,10 @@ function navigateReducer_noPPR(

if (
!applied &&
prefetchEntryCacheStatus === PrefetchCacheEntryStatus.stale
(prefetchEntryCacheStatus === PrefetchCacheEntryStatus.stale ||
// if we've navigated away from the global not found segment but didn't apply the flight data, we need to refetch
// as otherwise we'd be incorrectly using the global not found cache node for the incoming page
currentTree[0] === GLOBAL_NOT_FOUND_SEGMENT_KEY)
) {
applied = addRefetchToLeafSegments(
cache,
Expand Down Expand Up @@ -454,7 +460,10 @@ function navigateReducer_PPR(

if (
!applied &&
prefetchEntryCacheStatus === PrefetchCacheEntryStatus.stale
(prefetchEntryCacheStatus === PrefetchCacheEntryStatus.stale ||
// if we've navigated away from the global not found segment but didn't apply the flight data, we need to refetch
// as otherwise we'd be incorrectly using the global not found cache node for the incoming page
currentTree[0] === GLOBAL_NOT_FOUND_SEGMENT_KEY)
) {
applied = addRefetchToLeafSegments(
cache,
Expand Down
11 changes: 11 additions & 0 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ import {
usedDynamicAPIs,
createPostponedAbortSignal,
} from './dynamic-rendering'
import { GLOBAL_NOT_FOUND_SEGMENT_KEY } from '../../shared/lib/segment'

export type GetDynamicParamFromSegment = (
// [slug] / [[slug]] / [...slug]
Expand Down Expand Up @@ -405,6 +406,16 @@ async function ReactServerApp({ tree, ctx, asNotFound }: ReactServerAppProps) {
query
)

// If the page we're rendering is being treated as the global not-found page, we want to special-case
// the segment key so it doesn't collide with a page matching the same path.
// This is necessary because when rendering the global not-found, it will always be the root segment.
// If the not-found page prefetched a link to the root page, it would have the same data path
// (e.g., ['', { children: ['__PAGE__', {}] }]). Without this disambiguation, the router would interpret
// these pages as being able to share the same cache nodes, which is not the case as they render different things.
if (asNotFound) {
initialTree[0] = GLOBAL_NOT_FOUND_SEGMENT_KEY
}

const [MetadataTree, MetadataOutlet] = createMetadataComponents({
tree,
errorType: asNotFound ? 'not-found' : undefined,
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/shared/lib/segment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export function isGroupSegment(segment: string) {

export const PAGE_SEGMENT_KEY = '__PAGE__'
export const DEFAULT_SEGMENT_KEY = '__DEFAULT__'
export const GLOBAL_NOT_FOUND_SEGMENT_KEY = '__NOT_FOUND__'
19 changes: 19 additions & 0 deletions test/e2e/app-dir/prefetching-not-found/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// we want the layout to opt-out of static prefetching
export const dynamic = 'force-dynamic'

import Link from 'next/link'

export default function RootLayout({
children,
}: {
children: React.ReactNode
}) {
return (
<html lang="en">
<body>
<Link href="/">Link to `/`</Link>
<div>{children}</div>
</body>
</html>
)
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/prefetching-not-found/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function Home() {
return (
<div>
<h1>Home Page</h1>
<Link href="/fake-link">Go to Invalid Page</Link>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { nextTestSetup } from 'e2e-utils'
import { retry } from 'next-test-utils'

describe('prefetching-not-found', () => {
const { next } = nextTestSetup({
files: __dirname,
})

it('should correctly navigate to/from a global 404 page when following links with prefetch=auto', async () => {
let browser = await next.browser('/')
expect(await browser.elementByCss('h1').text()).toBe('Home Page')

await browser.elementByCss("[href='/fake-link']").click()

await retry(async () => {
expect(await browser.elementByCss('body').text()).toContain(
'This page could not be found.'
)
})

await browser.elementByCss("[href='/']").click()

await retry(async () => {
expect(await browser.elementByCss('h1').text()).toBe('Home Page')
})

// assert the same behavior, but starting at the not found page. This is to ensure that when we seed the prefetch cache,
// we don't have any cache collisions that would cause the not-found page to remain rendered when following a link to the home page
browser = await next.browser('/fake-link')
expect(await browser.elementByCss('body').text()).toContain(
'This page could not be found.'
)

await browser.elementByCss("[href='/']").click()

await retry(async () => {
expect(await browser.elementByCss('h1').text()).toBe('Home Page')
})
})
})

0 comments on commit fffa4c3

Please sign in to comment.