Skip to content

Commit

Permalink
[PPR Nav] Fix: Page data should always be applied (#60242)
Browse files Browse the repository at this point in the history
This fixes a case in the PPR navigations implementation where page data
was not being applied.

During a navigation, we compare the route trees of the old and new pages
to determine which layouts are shared. If the segment keys of two
layouts are the same, they are reused.

However, the server doesn't bother to assign segment keys to the leaf
segments (which we refer to as "page" segments) because they are never
part of a shared layout. It assigns all of them a special constant
(`__PAGE__`).

In the PPR implementation, I overlooked this and compared the segment
keys of all segments, including pages, not just shared layouts. So if
the only thing that changed during a navigation was the page data, and
not any parent layout, the client would fail to apply the navigation.

The fix is to add a special case for page segments before comparing
nested layouts. I also moved an existing special case for default pages,
since those are also leaf segments and are conceptually similar.
  • Loading branch information
acdlite authored Jan 4, 2024
1 parent da2e56a commit 8ae1167
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import type {
ChildSegmentMap,
ReadyCacheNode,
} from '../../../shared/lib/app-router-context.shared-runtime'
import { DEFAULT_SEGMENT_KEY } from '../../../shared/lib/segment'
import {
DEFAULT_SEGMENT_KEY,
PAGE_SEGMENT_KEY,
} from '../../../shared/lib/segment'
import { matchSegment } from '../match-segments'
import { createRouterCacheKey } from './create-router-cache-key'
import type { FetchServerResponseResult } from './fetch-server-response'
Expand Down Expand Up @@ -108,13 +111,50 @@ export function updateCacheNodeOnNavigation(
const newSegmentChild = newRouterStateChild[0]
const newSegmentKeyChild = createRouterCacheKey(newSegmentChild)

const oldSegmentChild =
oldRouterStateChild !== undefined ? oldRouterStateChild[0] : undefined

const oldCacheNodeChild =
oldSegmentMapChild !== undefined
? oldSegmentMapChild.get(newSegmentKeyChild)
: undefined

let taskChild: Task | null
if (matchSegment(newSegmentChild, oldRouterStateChild[0])) {
if (newSegmentChild === PAGE_SEGMENT_KEY) {
// This is a leaf segment — a page, not a shared layout. We always apply
// its data.
taskChild = spawnPendingTask(
newRouterStateChild,
prefetchDataChild !== undefined ? prefetchDataChild : null,
prefetchHead,
isPrefetchStale
)
} else if (newSegmentChild === DEFAULT_SEGMENT_KEY) {
// This is another kind of leaf segment — a default route.
//
// Default routes have special behavior. When there's no matching segment
// for a parallel route, Next.js preserves the currently active segment
// during a client navigation — but not for initial render. The server
// leaves it to the client to account for this. So we need to handle
// it here.
if (oldRouterStateChild !== undefined) {
// Reuse the existing Router State for this segment. We spawn a "task"
// just to keep track of the updated router state; unlike most, it's
// already fulfilled and won't be affected by the dynamic response.
taskChild = spawnReusedTask(oldRouterStateChild)
} else {
// There's no currently active segment. Switch to the "create" path.
taskChild = spawnPendingTask(
newRouterStateChild,
prefetchDataChild !== undefined ? prefetchDataChild : null,
prefetchHead,
isPrefetchStale
)
}
} else if (
oldSegmentChild !== undefined &&
matchSegment(newSegmentChild, oldSegmentChild)
) {
if (
oldCacheNodeChild !== undefined &&
oldRouterStateChild !== undefined
Expand Down Expand Up @@ -150,27 +190,13 @@ export function updateCacheNodeOnNavigation(
)
}
} else {
// The segment does not match.
if (newSegmentChild === DEFAULT_SEGMENT_KEY) {
// This is a special case related to default routes. When there's no
// matching segment for a parallel route, Next.js preserves the
// currently active segment during a client navigation — but not for
// initial render. The server leaves it to the client to account for
// this. So we need to handle it here.
//
// Reuse the existing Router State for this segment. We spawn a "task"
// just to keep track of the updated router state; unlike most, it's
// already fulfilled and won't be affected by the dynamic response.
taskChild = spawnReusedTask(oldRouterStateChild)
} else {
// This is a new tree. Switch to the "create" path.
taskChild = spawnPendingTask(
newRouterStateChild,
prefetchDataChild !== undefined ? prefetchDataChild : null,
prefetchHead,
isPrefetchStale
)
}
// This is a new tree. Switch to the "create" path.
taskChild = spawnPendingTask(
newRouterStateChild,
prefetchDataChild !== undefined ? prefetchDataChild : null,
prefetchHead,
isPrefetchStale
)
}

if (taskChild !== null) {
Expand Down
21 changes: 21 additions & 0 deletions test/e2e/app-dir/ppr-navigations/ppr-navigations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,27 @@ describe('ppr-navigations', () => {
expect(await dynamicContainer.innerText()).toBe('Some data [dynamic]')
}
)

test(
'updates page data during a nav even if no shared layouts have changed ' +
'(e.g. updating a search param on the current page)',
async () => {
next = await createNext({
files: __dirname + '/search-params',
})
const browser = await next.browser('/')

// Click a link that updates the current page's search params.
const link = await browser.elementByCss('a')
await link.click()

// Confirm that the page re-rendered with the new search params.
const searchParamsContainer = await browser.elementById('search-params')
expect(await searchParamsContainer.innerText()).toBe(
'Search params: {"blazing":"good"}'
)
}
)
})

// NOTE: I've intentionally not yet moved these helpers into a separate
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/app-dir/ppr-navigations/search-params/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function Root({ children }) {
return (
<html>
<body>{children}</body>
</html>
)
}
19 changes: 19 additions & 0 deletions test/e2e/app-dir/ppr-navigations/search-params/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import Link from 'next/link'

export default async function Page({
searchParams,
}: {
searchParams: { [key: string]: string | string[] | undefined }
}) {
const hasParams = Object.keys(searchParams).length > 0
return (
<>
<Link href="/?blazing=good">Go</Link>
{hasParams ? (
<div id="search-params">
Search params: {JSON.stringify(searchParams)}
</div>
) : null}
</>
)
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/ppr-navigations/search-params/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
ppr: true,
},
}

module.exports = nextConfig

0 comments on commit 8ae1167

Please sign in to comment.