From 72d48ea3741b0c67be825bf65bbd39f883ebde05 Mon Sep 17 00:00:00 2001 From: Zack Tanner Date: Wed, 13 Mar 2024 16:09:42 -0700 Subject: [PATCH] Fix interception/detail routes being triggered by router refreshes --- .../reducers/fast-refresh-reducer.ts | 7 +- .../has-interception-route-in-current-tree.ts | 28 ++++++++ .../reducers/refresh-reducer.ts | 8 ++- .../reducers/server-action-reducer.ts | 14 ++-- .../server/app-render/get-segment-param.tsx | 6 +- .../get-short-dynamic-param-type.tsx | 2 + packages/next/src/server/app-render/types.ts | 11 ++- .../src/server/dev/on-demand-entry-handler.ts | 2 + .../app/@interception/(.)detail-page/page.tsx | 7 ++ .../@interception2/(.)[...dynamic]/page.tsx | 16 +++++ .../app/catchall/@interception2/default.tsx | 3 + .../app/catchall/[...dynamic]/page.tsx | 16 +++++ .../app/catchall/default.tsx | 3 + .../app/catchall/layout.tsx | 16 +++++ .../app/catchall/page.tsx | 9 +++ .../app/detail-page/page.tsx | 15 ++++ .../@interception2/(.)[dynamic]/page.tsx | 16 +++++ .../app/dynamic/@interception2/default.tsx | 3 + .../app/dynamic/[dynamic]/page.tsx | 16 +++++ .../app/dynamic/default.tsx | 3 + .../app/dynamic/layout.tsx | 16 +++++ .../app/dynamic/page.tsx | 9 +++ .../parallel-routes-revalidation/app/page.tsx | 1 + .../parallel-routes-revalidation.test.ts | 68 ++++++++++++++++++- 24 files changed, 280 insertions(+), 15 deletions(-) create mode 100644 packages/next/src/client/components/router-reducer/reducers/has-interception-route-in-current-tree.ts create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/@interception/(.)detail-page/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/(.)[...dynamic]/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/default.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/catchall/[...dynamic]/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/catchall/default.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/catchall/layout.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/catchall/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/detail-page/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/(.)[dynamic]/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/default.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/[dynamic]/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/default.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/layout.tsx create mode 100644 test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/page.tsx diff --git a/packages/next/src/client/components/router-reducer/reducers/fast-refresh-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/fast-refresh-reducer.ts index 127da795505ab..716624e5e5873 100644 --- a/packages/next/src/client/components/router-reducer/reducers/fast-refresh-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/fast-refresh-reducer.ts @@ -14,6 +14,7 @@ import { applyFlightData } from '../apply-flight-data' import type { CacheNode } from '../../../../shared/lib/app-router-context.shared-runtime' import { createEmptyCacheNode } from '../../app-router' import { handleSegmentMismatch } from '../handle-segment-mismatch' +import { hasInterceptionRouteInCurrentTree } from './has-interception-route-in-current-tree' // A version of refresh reducer that keeps the cache around instead of wiping all of it. function fastRefreshReducerImpl( @@ -27,12 +28,16 @@ function fastRefreshReducerImpl( mutable.preserveCustomHistoryState = false const cache: CacheNode = createEmptyCacheNode() + // If the current tree was intercepted, the nextUrl should be included in the request. + // This is to ensure that the refresh request doesn't get intercepted, accidentally triggering the interception route. + const includeNextUrl = hasInterceptionRouteInCurrentTree(state.tree) + // TODO-APP: verify that `href` is not an external url. // Fetch data from the root of the tree. cache.lazyData = fetchServerResponse( new URL(href, origin), [state.tree[0], state.tree[1], state.tree[2], 'refetch'], - state.nextUrl, + includeNextUrl ? state.nextUrl : null, state.buildId ) diff --git a/packages/next/src/client/components/router-reducer/reducers/has-interception-route-in-current-tree.ts b/packages/next/src/client/components/router-reducer/reducers/has-interception-route-in-current-tree.ts new file mode 100644 index 0000000000000..636e0acb320fe --- /dev/null +++ b/packages/next/src/client/components/router-reducer/reducers/has-interception-route-in-current-tree.ts @@ -0,0 +1,28 @@ +import type { FlightRouterState } from '../../../../server/app-render/types' +import { isInterceptionRouteAppPath } from '../../../../server/future/helpers/interception-routes' + +export function hasInterceptionRouteInCurrentTree([ + segment, + parallelRoutes, +]: FlightRouterState): boolean { + // If we have a dynamic segment, it's marked as an interception route by the presence of the `i` suffix. + if (Array.isArray(segment) && (segment[2] === 'di' || segment[2] === 'ci')) { + return true + } + + // If segment is not an array, apply the existing string-based check + if (typeof segment === 'string' && isInterceptionRouteAppPath(segment)) { + return true + } + + // Iterate through parallelRoutes if they exist + if (parallelRoutes) { + for (const key in parallelRoutes) { + if (hasInterceptionRouteInCurrentTree(parallelRoutes[key])) { + return true + } + } + } + + return false +} diff --git a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts index d5a0bb15c95cd..7c2a9ed7ceb0f 100644 --- a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts @@ -14,6 +14,7 @@ import type { CacheNode } from '../../../../shared/lib/app-router-context.shared import { fillLazyItemsTillLeafWithHead } from '../fill-lazy-items-till-leaf-with-head' import { createEmptyCacheNode } from '../../app-router' import { handleSegmentMismatch } from '../handle-segment-mismatch' +import { hasInterceptionRouteInCurrentTree } from './has-interception-route-in-current-tree' export function refreshReducer( state: ReadonlyReducerState, @@ -28,12 +29,17 @@ export function refreshReducer( mutable.preserveCustomHistoryState = false const cache: CacheNode = createEmptyCacheNode() + + // If the current tree was intercepted, the nextUrl should be included in the request. + // This is to ensure that the refresh request doesn't get intercepted, accidentally triggering the interception route. + const includeNextUrl = hasInterceptionRouteInCurrentTree(state.tree) + // TODO-APP: verify that `href` is not an external url. // Fetch data from the root of the tree. cache.lazyData = fetchServerResponse( new URL(href, origin), [currentTree[0], currentTree[1], currentTree[2], 'refetch'], - state.nextUrl, + includeNextUrl ? state.nextUrl : null, state.buildId ) diff --git a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts index 02a2281ae17b9..387382be0365b 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts @@ -37,7 +37,7 @@ import type { CacheNode } from '../../../../shared/lib/app-router-context.shared import { handleMutable } from '../handle-mutable' import { fillLazyItemsTillLeafWithHead } from '../fill-lazy-items-till-leaf-with-head' import { createEmptyCacheNode } from '../../app-router' -import { extractPathFromFlightRouterState } from '../compute-changed-path' +import { hasInterceptionRouteInCurrentTree } from './has-interception-route-in-current-tree' import { handleSegmentMismatch } from '../handle-segment-mismatch' type FetchServerActionResult = { @@ -57,12 +57,12 @@ async function fetchServerAction( ): Promise { const body = await encodeReply(actionArgs) - const newNextUrl = extractPathFromFlightRouterState(state.tree) - // only pass along the `nextUrl` param (used for interception routes) if it exists and - // if it's different from the current `nextUrl`. This indicates the route has already been intercepted, - // and so the action should be as well. Otherwise the server action might be intercepted - // with the wrong action id (ie, one that corresponds with the intercepted route) - const includeNextUrl = state.nextUrl && state.nextUrl !== newNextUrl + // only pass along the `nextUrl` param (used for interception routes) if it exists in the current tree. + // This indicates the route has already been intercepted, and so the action should be as well. + // Otherwise the server action might be intercepted with the wrong action id + // (ie, one that corresponds with the intercepted route) + const includeNextUrl = + state.nextUrl && hasInterceptionRouteInCurrentTree(state.tree) const res = await fetch('', { method: 'POST', diff --git a/packages/next/src/server/app-render/get-segment-param.tsx b/packages/next/src/server/app-render/get-segment-param.tsx index e609aced06a0d..da09d770f1f07 100644 --- a/packages/next/src/server/app-render/get-segment-param.tsx +++ b/packages/next/src/server/app-render/get-segment-param.tsx @@ -20,6 +20,8 @@ export function getSegmentParam(segment: string): { if (segment.startsWith('[[...') && segment.endsWith(']]')) { return { + // TODO-APP: Optional catchall does not currently work with parallel routes, + // so for now aren't handling a potential interception marker. type: 'optional-catchall', param: segment.slice(5, -2), } @@ -27,14 +29,14 @@ export function getSegmentParam(segment: string): { if (segment.startsWith('[...') && segment.endsWith(']')) { return { - type: 'catchall', + type: interceptionMarker ? 'catchall-intercepted' : 'catchall', param: segment.slice(4, -1), } } if (segment.startsWith('[') && segment.endsWith(']')) { return { - type: 'dynamic', + type: interceptionMarker ? 'dynamic-intercepted' : 'dynamic', param: segment.slice(1, -1), } } diff --git a/packages/next/src/server/app-render/get-short-dynamic-param-type.tsx b/packages/next/src/server/app-render/get-short-dynamic-param-type.tsx index 670fedbdbb171..4f3c82057728c 100644 --- a/packages/next/src/server/app-render/get-short-dynamic-param-type.tsx +++ b/packages/next/src/server/app-render/get-short-dynamic-param-type.tsx @@ -5,8 +5,10 @@ export const dynamicParamTypes: Record< DynamicParamTypesShort > = { catchall: 'c', + 'catchall-intercepted': 'ci', 'optional-catchall': 'oc', dynamic: 'd', + 'dynamic-intercepted': 'di', } /** diff --git a/packages/next/src/server/app-render/types.ts b/packages/next/src/server/app-render/types.ts index 2f8bf9b28e692..94b6eb1ae321d 100644 --- a/packages/next/src/server/app-render/types.ts +++ b/packages/next/src/server/app-render/types.ts @@ -9,9 +9,14 @@ import type { SwrDelta } from '../lib/revalidate' import s from 'next/dist/compiled/superstruct' -export type DynamicParamTypes = 'catchall' | 'optional-catchall' | 'dynamic' - -const dynamicParamTypesSchema = s.enums(['c', 'oc', 'd']) +export type DynamicParamTypes = + | 'catchall' + | 'catchall-intercepted' + | 'optional-catchall' + | 'dynamic' + | 'dynamic-intercepted' + +const dynamicParamTypesSchema = s.enums(['c', 'ci', 'oc', 'd', 'di']) export type DynamicParamTypesShort = s.Infer diff --git a/packages/next/src/server/dev/on-demand-entry-handler.ts b/packages/next/src/server/dev/on-demand-entry-handler.ts index 4644089774e83..00e90e6db9129 100644 --- a/packages/next/src/server/dev/on-demand-entry-handler.ts +++ b/packages/next/src/server/dev/on-demand-entry-handler.ts @@ -82,10 +82,12 @@ function convertDynamicParamTypeToSyntax( ) { switch (dynamicParamTypeShort) { case 'c': + case 'ci': return `[...${param}]` case 'oc': return `[[...${param}]]` case 'd': + case 'di': return `[${param}]` default: throw new Error('Unknown dynamic param type') diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/@interception/(.)detail-page/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/@interception/(.)detail-page/page.tsx new file mode 100644 index 0000000000000..a789da028c826 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/@interception/(.)detail-page/page.tsx @@ -0,0 +1,7 @@ +export default function Page() { + return ( +
+

Detail Page (Intercepted)

+
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/(.)[...dynamic]/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/(.)[...dynamic]/page.tsx new file mode 100644 index 0000000000000..47dcf3aa0177a --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/(.)[...dynamic]/page.tsx @@ -0,0 +1,16 @@ +'use client' +import { useRouter } from 'next/navigation' + +export default function Page({ params }: { params: { dynamic: string } }) { + const router = useRouter() + return ( +
+

Detail Page (Intercepted)

+

{params.dynamic}

+ + {Math.random()} + + +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/default.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/default.tsx new file mode 100644 index 0000000000000..86b9e9a388129 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/@interception2/default.tsx @@ -0,0 +1,3 @@ +export default function Default() { + return null +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/[...dynamic]/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/[...dynamic]/page.tsx new file mode 100644 index 0000000000000..136c15f3b1782 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/[...dynamic]/page.tsx @@ -0,0 +1,16 @@ +'use client' +import { useRouter } from 'next/navigation' + +export default function Page({ params }: { params: { dynamic: string } }) { + const router = useRouter() + return ( +
+

Detail Page (Non-Intercepted)

+

{params.dynamic}

+ + {Math.random()} + + +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/default.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/default.tsx new file mode 100644 index 0000000000000..86b9e9a388129 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/default.tsx @@ -0,0 +1,3 @@ +export default function Default() { + return null +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/layout.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/layout.tsx new file mode 100644 index 0000000000000..0fd3e7e553c5e --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/layout.tsx @@ -0,0 +1,16 @@ +import React from 'react' + +export default function Layout({ + children, + interception2, +}: { + children: React.ReactNode + interception2: React.ReactNode +}) { + return ( +
+ {children} + {interception2} +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/page.tsx new file mode 100644 index 0000000000000..90a4e912dd438 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/catchall/page.tsx @@ -0,0 +1,9 @@ +import Link from 'next/link' + +export default function Page() { + return ( +
+ Dynamic Catchall Interception +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/detail-page/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/detail-page/page.tsx new file mode 100644 index 0000000000000..aa12bd1c69834 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/detail-page/page.tsx @@ -0,0 +1,15 @@ +'use client' +import { useRouter } from 'next/navigation' + +export default function Page() { + const router = useRouter() + return ( +
+

Detail Page (Non-Intercepted)

+ + {Math.random()} + + +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/(.)[dynamic]/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/(.)[dynamic]/page.tsx new file mode 100644 index 0000000000000..47dcf3aa0177a --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/(.)[dynamic]/page.tsx @@ -0,0 +1,16 @@ +'use client' +import { useRouter } from 'next/navigation' + +export default function Page({ params }: { params: { dynamic: string } }) { + const router = useRouter() + return ( +
+

Detail Page (Intercepted)

+

{params.dynamic}

+ + {Math.random()} + + +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/default.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/default.tsx new file mode 100644 index 0000000000000..86b9e9a388129 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/@interception2/default.tsx @@ -0,0 +1,3 @@ +export default function Default() { + return null +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/[dynamic]/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/[dynamic]/page.tsx new file mode 100644 index 0000000000000..136c15f3b1782 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/[dynamic]/page.tsx @@ -0,0 +1,16 @@ +'use client' +import { useRouter } from 'next/navigation' + +export default function Page({ params }: { params: { dynamic: string } }) { + const router = useRouter() + return ( +
+

Detail Page (Non-Intercepted)

+

{params.dynamic}

+ + {Math.random()} + + +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/default.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/default.tsx new file mode 100644 index 0000000000000..86b9e9a388129 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/default.tsx @@ -0,0 +1,3 @@ +export default function Default() { + return null +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/layout.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/layout.tsx new file mode 100644 index 0000000000000..0fd3e7e553c5e --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/layout.tsx @@ -0,0 +1,16 @@ +import React from 'react' + +export default function Layout({ + children, + interception2, +}: { + children: React.ReactNode + interception2: React.ReactNode +}) { + return ( +
+ {children} + {interception2} +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/page.tsx new file mode 100644 index 0000000000000..de2355977743c --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/dynamic/page.tsx @@ -0,0 +1,9 @@ +import Link from 'next/link' + +export default function Page() { + return ( +
+ Dynamic Interception +
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-revalidation/app/page.tsx b/test/e2e/app-dir/parallel-routes-revalidation/app/page.tsx index ed113546ee172..b8f647cca09e0 100644 --- a/test/e2e/app-dir/parallel-routes-revalidation/app/page.tsx +++ b/test/e2e/app-dir/parallel-routes-revalidation/app/page.tsx @@ -12,6 +12,7 @@ export default async function Home() { Open Revalidate Modal Open Refresh Modal Open Redirect Modal + Intercepted Detail Page
{randomNumber}

Current Data

diff --git a/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts b/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts index f8890a42903fe..c62efbc81b8b2 100644 --- a/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts +++ b/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts @@ -1,5 +1,5 @@ import { createNextDescribe } from 'e2e-utils' -import { check } from 'next-test-utils' +import { check, retry } from 'next-test-utils' createNextDescribe( 'parallel-routes-revalidation', @@ -79,5 +79,71 @@ createNextDescribe( await check(() => browser.hasElementByCssSelector('#redirect'), false) await check(() => browser.elementByCss('body').text(), /Current Data/) }) + + it.each([ + { path: '/detail-page' }, + { path: '/dynamic/foobar', param: 'foobar' }, + { path: '/catchall/foobar', param: 'foobar' }, + ])( + 'should not trigger interception when calling router.refresh() on an intercepted route ($path)', + async (route) => { + const browser = await next.browser(route.path) + expect(await browser.elementById('detail-title').text()).toBe( + 'Detail Page (Non-Intercepted)' + ) + const randomNumber = (await browser.elementById('random-number')).text() + if (route.param) { + expect(await browser.elementById('params').text()).toBe('foobar') + } + + await browser.elementByCss('button').click() + await retry(async () => { + expect(await browser.elementById('detail-title').text()).toBe( + 'Detail Page (Non-Intercepted)' + ) + const newRandomNumber = await browser + .elementById('random-number') + .text() + + expect(randomNumber).not.toBe(newRandomNumber) + + if (route.param) { + expect(await browser.elementById('params').text()).toBe('foobar') + } + }) + } + ) + + it.each([{ path: '/dynamic', param: 'foobar' }])( + 'should not trigger full page when calling router.refresh() on an intercepted route ($path)', + async (route) => { + const browser = await next.browser(route.path) + await browser.elementByCss('a').click() + + expect(await browser.elementById('detail-title').text()).toBe( + 'Detail Page (Intercepted)' + ) + const randomNumber = (await browser.elementById('random-number')).text() + if (route.param) { + expect(await browser.elementById('params').text()).toBe('foobar') + } + + await browser.elementByCss('button').click() + await retry(async () => { + expect(await browser.elementById('detail-title').text()).toBe( + 'Detail Page (Intercepted)' + ) + const newRandomNumber = await browser + .elementById('random-number') + .text() + + expect(randomNumber).not.toBe(newRandomNumber) + + if (route.param) { + expect(await browser.elementById('params').text()).toBe('foobar') + } + }) + } + ) } )