Skip to content

Commit

Permalink
fix interception route refresh behavior with dynamic params (#64006)
Browse files Browse the repository at this point in the history
### What
When triggering an interception route that has a parent with dynamic
params, and then later going to "refresh" the tree, either by calling
`router.refresh` or revalidating in a server action, the refresh action
would silently fail and the router would be in a bad state.

### Why
Because of the dependency that interception routes currently have on
`FlightRouterState` for dynamic params extraction, we need to make sure
the refetch has the full tree so that it can properly extract earlier
params. Since the refreshing logic traversed parallel routes and scoped
the refresh to that particular segment, it would skip over earlier
segments, and so when the server attempted to diff the tree, it would
return an updated tree that corresponded with the wrong segment
(`[locale]` rather than `["locale", "en", "d]`).

Separately, since a page segment might be `__PAGE__?{"locale": "en"}`
rather than just `__PAGE__`, this updates the refetch marker logic to do
a partial match on the page segment key.

### How
This keeps a reference to the root of the updated tree so that the
refresh always starts at the top. This has the side effect of
re-rendering more data when making the "stale" refetch request, but this
is necessary until we can decouple `FlightRouterState` from interception
routes.

shout-out to @steve-marmalade for helping find this bug and providing
excellent Replays to help track it down 🙏

x-ref:

- #63900

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->


Closes NEXT-2986
  • Loading branch information
ztanner authored Apr 3, 2024
1 parent 6a1e70a commit ad98cda
Show file tree
Hide file tree
Showing 14 changed files with 179 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ export async function refreshInactiveParallelSegments(
options: RefreshInactiveParallelSegments
) {
const fetchedSegments = new Set<string>()
await refreshInactiveParallelSegmentsImpl({ ...options, fetchedSegments })
await refreshInactiveParallelSegmentsImpl({
...options,
rootTree: options.updatedTree,
fetchedSegments,
})
}

async function refreshInactiveParallelSegmentsImpl({
Expand All @@ -36,7 +40,11 @@ async function refreshInactiveParallelSegmentsImpl({
updatedCache,
includeNextUrl,
fetchedSegments,
}: RefreshInactiveParallelSegments & { fetchedSegments: Set<string> }) {
rootTree = updatedTree,
}: RefreshInactiveParallelSegments & {
fetchedSegments: Set<string>
rootTree: FlightRouterState
}) {
const [, parallelRoutes, refetchPathname, refetchMarker] = updatedTree
const fetchPromises = []

Expand All @@ -56,7 +64,9 @@ async function refreshInactiveParallelSegmentsImpl({
// we capture the pathname of the refetch without search params, so that it can be refetched with
// the "latest" search params when it comes time to actually trigger the fetch (below)
new URL(refetchPathname + location.search, location.origin),
[updatedTree[0], updatedTree[1], updatedTree[2], 'refetch'],
// 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'],
includeNextUrl ? state.nextUrl : null,
state.buildId
).then((fetchResponse) => {
Expand Down Expand Up @@ -85,6 +95,7 @@ async function refreshInactiveParallelSegmentsImpl({
updatedCache,
includeNextUrl,
fetchedSegments,
rootTree,
})

fetchPromises.push(parallelFetchPromise)
Expand All @@ -104,7 +115,8 @@ export function addRefreshMarkerToActiveParallelSegments(
pathname: string
) {
const [segment, parallelRoutes, , refetchMarker] = tree
if (segment === PAGE_SEGMENT_KEY && refetchMarker !== 'refresh') {
// 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[3] = 'refresh'
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use client'
import { useRouter } from 'next/navigation'

export function Button() {
export function RefreshButton() {
const router = useRouter()

return (
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use client'
import { revalidateAction } from '../nested-revalidate/@modal/modal/action'

export function RevalidateButton({ id }: { id?: string }) {
return (
<button
id={`revalidate-button${id ? `-${id}` : ''}`}
style={{ color: 'orange', padding: '10px' }}
onClick={() => revalidateAction()}
>
Revalidate
</button>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { RefreshButton } from '../../../../components/RefreshButton'
import { RevalidateButton } from '../../../../components/RevalidateButton'

const getRandom = async () => Math.random()

export default async function Page({ params }) {
const someProp = await getRandom()

return (
<dialog open>
<div>{params.dynamic}</div>
<div style={{ display: 'flex', flexDirection: 'column' }}>
<div>
<span>Modal Page</span>
<span id="modal-random">{someProp}</span>
</div>
<RefreshButton />
<RevalidateButton />
</div>
</dialog>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Default() {
return null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import Link from 'next/link'

export const dynamic = 'force-dynamic'

export default function Layout({
children,
modal,
}: {
children: React.ReactNode
modal: React.ReactNode
}) {
return (
<div>
<div>{children}</div>
<div>{modal}</div>
<Link href="/dynamic-refresh/foo/other">Go to Other Page</Link>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { RefreshButton } from '../../../components/RefreshButton'

export default function Page() {
return (
<>
<span>Login Page</span>
<RefreshButton />
Random Number: <span id="login-page-random">{Math.random()}</span>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { RefreshButton } from '../../../components/RefreshButton'
import { RevalidateButton } from '../../../components/RevalidateButton'

export default function Page() {
return (
<div>
<div>Other Page</div>
<div id="other-page-random">{Math.random()}</div>
<RefreshButton />
<RevalidateButton id="other" />
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Link from 'next/link'

export default function Home() {
return (
<main>
<Link href="/dynamic-refresh/foo/login">
<button>Login button</button>
</Link>
<div>
Random # from Root Page: <span id="random-number">{Math.random()}</span>
</div>
</main>
)
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Button } from '../../buttonRefresh'
import { RefreshButton } from '../../../components/RefreshButton'
import { RevalidateButton } from '../../../components/RevalidateButton'

const getRandom = async () => Math.random()

Expand All @@ -12,7 +13,8 @@ export default async function Page() {
<span>Modal Page</span>
<span id="modal-random">{someProp}</span>
</div>
<Button />
<RefreshButton />
<RevalidateButton />
</div>
</dialog>
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Button } from '../buttonRefresh'
import { RefreshButton } from '../../components/RefreshButton'

export default function Page() {
return (
<>
<span>Login Page</span>
<Button />
<RefreshButton />
Random Number: <span id="login-page-random">{Math.random()}</span>
</>
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { RefreshButton } from '../../components/RefreshButton'
import { RevalidateButton } from '../../components/RevalidateButton'

export default function Page() {
return (
<div>
<div>Other Page</div>
<div id="other-page-random">{Math.random()}</div>
<RefreshButton />
<RevalidateButton id="other" />
</div>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -158,21 +158,42 @@ createNextDescribe(
})
})

describe('router.refresh', () => {
describe.each([
{ basePath: '/refreshing', label: 'regular' },
{ basePath: '/dynamic-refresh/foo', label: 'dynamic' },
])('router.refresh ($label)', ({ basePath }) => {
it('should correctly refresh data for the intercepted route and previously active page slot', async () => {
const browser = await next.browser('/refreshing')
const initialRandomNumber = await browser.elementById('random-number')
const browser = await next.browser(basePath)
let initialRandomNumber = await browser.elementById('random-number')

await browser.elementByCss("[href='/refreshing/login']").click()
await browser.elementByCss(`[href='${basePath}/login']`).click()

// interception modal should be visible
const initialModalRandomNumber = await browser
let initialModalRandomNumber = await browser
.elementById('modal-random')
.text()

// trigger a refresh
await browser.elementById('refresh-button').click()

await retry(async () => {
const newRandomNumber = await browser
.elementById('random-number')
.text()
const newModalRandomNumber = await browser
.elementById('modal-random')
.text()
expect(initialRandomNumber).not.toBe(newRandomNumber)
expect(initialModalRandomNumber).not.toBe(newModalRandomNumber)

// reset the initial values to be the new values, so that we can verify the revalidate case below.
initialRandomNumber = newRandomNumber
initialModalRandomNumber = newModalRandomNumber
})

// trigger a revalidate
await browser.elementById('revalidate-button').click()

await retry(async () => {
const newRandomNumber = await browser
.elementById('random-number')
Expand Down Expand Up @@ -206,25 +227,45 @@ createNextDescribe(
})

it('should correctly refresh data for previously intercepted modal and active page slot', async () => {
const browser = await next.browser('/refreshing')
const browser = await next.browser(basePath)

await browser.elementByCss("[href='/refreshing/login']").click()
await browser.elementByCss(`[href='${basePath}/login']`).click()

// interception modal should be visible
const initialModalRandomNumber = await browser
let initialModalRandomNumber = await browser
.elementById('modal-random')
.text()

await browser.elementByCss("[href='/refreshing/other']").click()
await browser.elementByCss(`[href='${basePath}/other']`).click()
// data for the /other page should be visible

const initialOtherPageRandomNumber = await browser
let initialOtherPageRandomNumber = await browser
.elementById('other-page-random')
.text()

// trigger a refresh
await browser.elementById('refresh-button').click()

await retry(async () => {
const newModalRandomNumber = await browser
.elementById('modal-random')
.text()

const newOtherPageRandomNumber = await browser
.elementById('other-page-random')
.text()
expect(initialModalRandomNumber).not.toBe(newModalRandomNumber)
expect(initialOtherPageRandomNumber).not.toBe(
newOtherPageRandomNumber
)
// reset the initial values to be the new values, so that we can verify the revalidate case below.
initialOtherPageRandomNumber = newOtherPageRandomNumber
initialModalRandomNumber = newModalRandomNumber
})

// trigger a revalidate
await browser.elementById('revalidate-button').click()

await retry(async () => {
const newModalRandomNumber = await browser
.elementById('modal-random')
Expand Down
6 changes: 4 additions & 2 deletions test/turbopack-build-tests-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -2271,8 +2271,10 @@
"parallel-routes-revalidation should handle a redirect action when called in a slot",
"parallel-routes-revalidation should handle router.refresh() when called in a slot",
"parallel-routes-revalidation should not trigger full page when calling router.refresh() on an intercepted route",
"parallel-routes-revalidation router.refresh should correctly refresh data for the intercepted route and previously active page slot",
"parallel-routes-revalidation router.refresh should correctly refresh data for previously intercepted modal and active page slot",
"parallel-routes-revalidation router.refresh (dynamic) should correctly refresh data for the intercepted route and previously active page slot",
"parallel-routes-revalidation router.refresh (dynamic) should correctly refresh data for previously intercepted modal and active page slot",
"parallel-routes-revalidation router.refresh (regular) should correctly refresh data for the intercepted route and previously active page slot",
"parallel-routes-revalidation router.refresh (regular) should correctly refresh data for previously intercepted modal and active page slot",
"parallel-routes-revalidation server action revalidation handles refreshing when multiple parallel slots are active"
],
"pending": [],
Expand Down

0 comments on commit ad98cda

Please sign in to comment.