Skip to content

Commit

Permalink
fix router prefetch cache key to work with route interception (#59861)
Browse files Browse the repository at this point in the history
### What?
When handling route interception in two different segments but handled
by the same interception route, the first interception will show the
correct component but attempting the same interception on another
segment will return elements from the first request, not the second.

### Why?
Prefetch cache entries are created from the browser URL. However, route
interception makes use of `nextUrl` to mask the underlying components
that are being fetched from the server to handle the request

Consider the following scenario:

```
app
   foo
     @modal
       (...)post
         [id]
   bar
     @modal
       (...post)
         [id]
   post
     [id]
     
```
If you trigger an interception on `/foo`, your URL is going to be masked
to `/post/1` and keyed as such in the prefetch cache. However, the cache
entry is actually associated with `app/foo/@modal/(...post)/[id]`. That
means when you trigger the same interception on `/bar`, it will return
the tree from `/foo`.

### How?
This PR will prefix the prefetch cache key with `state.nextUrl` when
necessary.

Fixes #49878
Fixes #52748
Closes NEXT-1818
  • Loading branch information
ztanner authored Dec 22, 2023
1 parent c4adae8 commit 6545783
Show file tree
Hide file tree
Showing 18 changed files with 239 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { addPathPrefix } from '../../../../shared/lib/router/utils/add-path-prefix'
import { pathHasPrefix } from '../../../../shared/lib/router/utils/path-has-prefix'
import { createHrefFromUrl } from '../create-href-from-url'

/**
* Creates a cache key for the router prefetch cache
*
* @param url - The URL being navigated to
* @param nextUrl - an internal URL, primarily used for handling rewrites. Defaults to '/'.
* @return The generated prefetch cache key.
*/
export function createPrefetchCacheKey(url: URL, nextUrl: string | null) {
const pathnameFromUrl = createHrefFromUrl(
url,
// Ensures the hash is not part of the cache key as it does not impact the server fetch
false
)

// delimit the prefix so we don't conflict with other pages
const nextUrlPrefix = `${nextUrl}%`

// Route interception depends on `nextUrl` values which aren't a 1:1 mapping to a URL
// The cache key that we store needs to use `nextUrl` to properly distinguish cache entries
if (nextUrl && !pathHasPrefix(pathnameFromUrl, nextUrl)) {
return addPathPrefix(pathnameFromUrl, nextUrlPrefix)
}

return pathnameFromUrl
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
listenForDynamicRequest,
updateCacheNodeOnNavigation,
} from '../ppr-navigations'
import { createPrefetchCacheKey } from './create-prefetch-cache-key'

export function handleExternalUrl(
state: ReadonlyReducerState,
Expand Down Expand Up @@ -126,7 +127,8 @@ function navigateReducer_noPPR(
return handleExternalUrl(state, mutable, url.toString(), pendingPush)
}

let prefetchValues = state.prefetchCache.get(createHrefFromUrl(url, false))
const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchValues = state.prefetchCache.get(prefetchCacheKey)

// If we don't have a prefetch value, we need to create one
if (!prefetchValues) {
Expand All @@ -152,7 +154,7 @@ function navigateReducer_noPPR(
lastUsedTime: null,
}

state.prefetchCache.set(createHrefFromUrl(url, false), newPrefetchValue)
state.prefetchCache.set(prefetchCacheKey, newPrefetchValue)
prefetchValues = newPrefetchValue
}

Expand Down Expand Up @@ -320,7 +322,8 @@ function navigateReducer_PPR(
return handleExternalUrl(state, mutable, url.toString(), pendingPush)
}

let prefetchValues = state.prefetchCache.get(createHrefFromUrl(url, false))
const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl)
let prefetchValues = state.prefetchCache.get(prefetchCacheKey)

// If we don't have a prefetch value, we need to create one
if (!prefetchValues) {
Expand All @@ -346,7 +349,7 @@ function navigateReducer_PPR(
lastUsedTime: null,
}

state.prefetchCache.set(createHrefFromUrl(url, false), newPrefetchValue)
state.prefetchCache.set(prefetchCacheKey, newPrefetchValue)
prefetchValues = newPrefetchValue
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { createHrefFromUrl } from '../create-href-from-url'
import { fetchServerResponse } from '../fetch-server-response'
import type {
PrefetchAction,
Expand All @@ -9,6 +8,7 @@ import { PrefetchKind } from '../router-reducer-types'
import { prunePrefetchCache } from './prune-prefetch-cache'
import { NEXT_RSC_UNION_QUERY } from '../../app-router-headers'
import { PromiseQueue } from '../../promise-queue'
import { createPrefetchCacheKey } from './create-prefetch-cache-key'

export const prefetchQueue = new PromiseQueue(5)

Expand All @@ -22,20 +22,16 @@ export function prefetchReducer(
const { url } = action
url.searchParams.delete(NEXT_RSC_UNION_QUERY)

const href = createHrefFromUrl(
url,
// Ensures the hash is not part of the cache key as it does not affect fetching the server
false
)
const prefetchCacheKey = createPrefetchCacheKey(url, state.nextUrl)
const cacheEntry = state.prefetchCache.get(prefetchCacheKey)

const cacheEntry = state.prefetchCache.get(href)
if (cacheEntry) {
/**
* If the cache entry present was marked as temporary, it means that we prefetched it from the navigate reducer,
* where we didn't have the prefetch intent. We want to update it to the new, more accurate, kind here.
*/
if (cacheEntry.kind === PrefetchKind.TEMPORARY) {
state.prefetchCache.set(href, {
state.prefetchCache.set(prefetchCacheKey, {
...cacheEntry,
kind: action.kind,
})
Expand Down Expand Up @@ -68,7 +64,7 @@ export function prefetchReducer(
)

// Create new tree based on the flightSegmentPath and router state patch
state.prefetchCache.set(href, {
state.prefetchCache.set(prefetchCacheKey, {
// Create new tree based on the flightSegmentPath and router state patch
treeAtTimeOfPrefetch: state.tree,
data: serverResponse,
Expand Down
27 changes: 27 additions & 0 deletions test/e2e/app-dir/interception-route-prefetch-cache/app/Modal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use client'

import { useRouter } from 'next/navigation'

interface ModalProps {
title: string
context: string
}

export function Modal({ title, context }: ModalProps) {
const router = useRouter()

return (
<div>
<div className="modal">
<h1>{title}</h1>
<h2>{context}</h2>
</div>
<div
className="modal-overlay"
onClick={() => {
router.back()
}}
/>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Modal } from '../../../../Modal'

export default function BarPagePostInterceptSlot({
params: { id },
}: {
params: {
id: string
}
}) {
return <Modal title={`Post ${id}`} context="Intercepted on Bar Page" />
}
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,3 @@
export default function Default() {
return null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export default function BarLayout({
modal,
children,
}: {
modal: React.ReactNode
children: React.ReactNode
}) {
return (
<>
<div id="slot">{modal}</div>
<div id="children">{children}</div>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function BarPage() {
return (
<div>
<h1>Bar Page</h1>
<Link href="/post/1">Post 1</Link>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Modal } from '../../../../Modal'

export default function FooPagePostInterceptSlot({
params: { id },
}: {
params: {
id: string
}
}) {
return <Modal title={`Post ${id}`} context="Intercepted on Foo Page" />
}
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,3 @@
export default function Default() {
return null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export default function FooLayout({
modal,
children,
}: {
modal: React.ReactNode
children: React.ReactNode
}) {
return (
<>
<div id="slot">{modal}</div>
<div id="children">{children}</div>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default function FooPage() {
return (
<div>
<h1>Foo Page</h1>
<Link href="/post/1">Post 1</Link>
</div>
)
}
13 changes: 13 additions & 0 deletions test/e2e/app-dir/interception-route-prefetch-cache/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import Link from 'next/link'

export default function RootLayout({ children }) {
return (
<html>
<head />
<body>
<Link href="/">home</Link>
{children}
</body>
</html>
)
}
18 changes: 18 additions & 0 deletions test/e2e/app-dir/interception-route-prefetch-cache/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import Link from 'next/link'

export default function Home() {
return (
<ul>
<li>
<Link href="/foo">foo</Link>
</li>
<li>
<Link href="/bar">bar</Link>
</li>
<br />
<li>
<Link href="/post/1">post 1</Link>
</li>
</ul>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export default function PostPage({
params: { id },
}: {
params: {
id: string
}
}) {
return (
<div>
<h1>Post {id}</h1>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { createNextDescribe } from 'e2e-utils'
import { check } from 'next-test-utils'

createNextDescribe(
'interception-route-prefetch-cache',
{
files: __dirname,
},
({ next }) => {
it('should render the correct interception when two distinct layouts share the same path structure', async () => {
const browser = await next.browser('/')

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

await check(() => browser.elementById('children').text(), /Foo Page/)

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

// Ensure the existing page content is still the same
await check(() => browser.elementById('children').text(), /Foo Page/)

// Verify we got the right interception
await check(
() => browser.elementById('slot').text(),
/Intercepted on Foo Page/
)

// Go back home. At this point, the router cache should have content from /foo
// Now we want to ensure that /bar doesn't share that same prefetch cache entry
await browser.elementByCss('[href="/"]').click()
await browser.elementByCss('[href="/bar"]').click()

await check(() => browser.elementById('children').text(), /Bar Page/)
await browser.elementByCss('[href="/post/1"]').click()

// Ensure the existing page content is still the same. If the prefetch cache resolved the wrong cache node
// then we'd see the content from /foo
await check(() => browser.elementById('children').text(), /Bar Page/)
await check(
() => browser.elementById('slot').text(),
/Intercepted on Bar Page/
)
})
}
)

0 comments on commit 6545783

Please sign in to comment.