From e4158f6cec77fc6afa9a072b2b81267fcb88e18c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Best?= Date: Tue, 14 Nov 2023 20:29:18 +0100 Subject: [PATCH] fix: Don't reset shallow URL updates on prefetch (#58297) ## Description Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced in vercel/next.js#56497 that turned the Redux store state into a Promise, rather than a synchronous state update. This caused the `sync` function -- used to send state updates to the Redux Devtools -- to be recreated on every dispatch, which in turn, by referential instability, caused the `HistoryUpdater` component to re-render and trigger a `history.replaceState` with no particular change, but with the internal `canonicalUrl`. When an app does a soft/shallow navigation by calling history methods directly (currently the only way to do shallow search params updates in the app router), these changes would have been overwritten by any prefetch (eg: hovering or mounting a Link), which is usually a no-op for the navigation state. This PR changes the `sync` function to take the state as an argument rather than as a closure. The whole app router state is also unwrapped only once, and fed to the HistoryUpdater. Changes to its contents made by reducers will cause the HistoryUpdater effect to re-run, triggering history updates and a call to the sync function. ## Context I maintain [`next-usequerystate`](https://github.com/47ng/next-usequerystate), which is used in the Vercel dashboard, and which is impacted by this change (see [#388](https://github.com/47ng/next-usequerystate/issues/388)). ## History @timneutkens introduced the `sync` function and the whole Redux devtools reducer in vercel/next.js#39866, with the note: > a new hook useReducerWithReduxDevtools has been added, we'll probably want to put this behind a compile-time flag when the new router is marked stable but until then it's useful to have it enabled by default (only when you have Redux Devtools installed ofcourse). If a different direction is needed to keep sending `RENDER_SYNC` actions to Redux devtools, I'll be happy to rework this PR to move the `sync` function into the action queue. ## Changes - [x] Added e2e test. Requires a `start` mode as prefetch links are disabled in development. Test was verified to fail from next@>=12.0.2-canary.7 without the fix. Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com> --- .../next/src/client/components/app-router.tsx | 22 ++++++++----------- .../components/use-reducer-with-devtools.ts | 18 ++++++++++----- .../app/search-params/shallow/other/page.js | 5 +++++ .../app/search-params/shallow/page.js | 19 ++++++++++++++++ .../e2e/app-dir/navigation/navigation.test.ts | 11 ++++++++++ 5 files changed, 57 insertions(+), 18 deletions(-) create mode 100644 test/e2e/app-dir/navigation/app/search-params/shallow/other/page.js create mode 100644 test/e2e/app-dir/navigation/app/search-params/shallow/page.js diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 13c6811c20017..d66dc65eb2f13 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -35,7 +35,7 @@ import { PrefetchKind, } from './router-reducer/router-reducer-types' import type { - PushRef, + AppRouterState, ReducerActions, RouterChangeByServerResponse, RouterNavigate, @@ -49,6 +49,7 @@ import { import { useReducerWithReduxDevtools, useUnwrapState, + type ReduxDevtoolsSyncFn, } from './use-reducer-with-devtools' import { ErrorBoundary } from './error-boundary' import { createInitialRouterState } from './router-reducer/create-initial-router-state' @@ -110,17 +111,14 @@ function isExternalURL(url: URL) { } function HistoryUpdater({ - tree, - pushRef, - canonicalUrl, + appRouterState, sync, }: { - tree: FlightRouterState - pushRef: PushRef - canonicalUrl: string - sync: () => void + appRouterState: AppRouterState + sync: ReduxDevtoolsSyncFn }) { useInsertionEffect(() => { + const { tree, pushRef, canonicalUrl } = appRouterState const historyState = { ...(process.env.__NEXT_WINDOW_HISTORY_SUPPORT && pushRef.preserveCustomHistoryState @@ -148,8 +146,8 @@ function HistoryUpdater({ originalReplaceState(historyState, '', canonicalUrl) } } - sync() - }, [tree, pushRef, canonicalUrl, sync]) + sync(appRouterState) + }, [appRouterState, sync]) return null } @@ -589,9 +587,7 @@ function Router({ return ( <> diff --git a/packages/next/src/client/components/use-reducer-with-devtools.ts b/packages/next/src/client/components/use-reducer-with-devtools.ts index 1db2769684db3..dd75194c63a3b 100644 --- a/packages/next/src/client/components/use-reducer-with-devtools.ts +++ b/packages/next/src/client/components/use-reducer-with-devtools.ts @@ -9,6 +9,8 @@ import { } from './router-reducer/router-reducer-types' import { ActionQueueContext } from '../../shared/lib/router/action-queue' +export type ReduxDevtoolsSyncFn = (state: AppRouterState) => void + function normalizeRouterState(val: any): any { if (val instanceof Map) { const obj: { [key: string]: any } = {} @@ -86,13 +88,13 @@ export function useUnwrapState(state: ReducerState): AppRouterState { function useReducerWithReduxDevtoolsNoop( initialState: AppRouterState -): [ReducerState, Dispatch, () => void] { +): [ReducerState, Dispatch, ReduxDevtoolsSyncFn] { return [initialState, () => {}, () => {}] } function useReducerWithReduxDevtoolsImpl( initialState: AppRouterState -): [ReducerState, Dispatch, () => void] { +): [ReducerState, Dispatch, ReduxDevtoolsSyncFn] { const [state, setState] = React.useState(initialState) const actionQueue = useContext(ActionQueueContext) @@ -149,14 +151,20 @@ function useReducerWithReduxDevtoolsImpl( [actionQueue, initialState] ) - const sync = useCallback(() => { + // Sync is called after a state update in the HistoryUpdater, + // for debugging purposes. Since the reducer state may be a Promise, + // we let the app router use() it and sync on the resolved value if + // something changed. + // Using the `state` here would be referentially unstable and cause + // undesirable re-renders and history updates. + const sync = useCallback((resolvedState) => { if (devtoolsConnectionRef.current) { devtoolsConnectionRef.current.send( { type: 'RENDER_SYNC' }, - normalizeRouterState(state) + normalizeRouterState(resolvedState) ) } - }, [state]) + }, []) return [state, dispatch, sync] } diff --git a/test/e2e/app-dir/navigation/app/search-params/shallow/other/page.js b/test/e2e/app-dir/navigation/app/search-params/shallow/other/page.js new file mode 100644 index 0000000000000..642a064d8fb5b --- /dev/null +++ b/test/e2e/app-dir/navigation/app/search-params/shallow/other/page.js @@ -0,0 +1,5 @@ +import React from 'react' + +export default function Page() { + return

Prefetch target page

+} diff --git a/test/e2e/app-dir/navigation/app/search-params/shallow/page.js b/test/e2e/app-dir/navigation/app/search-params/shallow/page.js new file mode 100644 index 0000000000000..9f012b0ffc2d7 --- /dev/null +++ b/test/e2e/app-dir/navigation/app/search-params/shallow/page.js @@ -0,0 +1,19 @@ +'use client' + +import React from 'react' +import Link from 'next/link' + +export default function Page() { + const setShallowSearchParams = React.useCallback(() => { + // Maintain history state, but set a shallow search param + history.replaceState(history.state, '', '?foo=bar') + }, []) + return ( + <> + + + Then hover me + + + ) +} diff --git a/test/e2e/app-dir/navigation/navigation.test.ts b/test/e2e/app-dir/navigation/navigation.test.ts index 4bf3238bb5308..4a568331ca0f6 100644 --- a/test/e2e/app-dir/navigation/navigation.test.ts +++ b/test/e2e/app-dir/navigation/navigation.test.ts @@ -55,6 +55,17 @@ createNextDescribe( }, 'success') }) + it('should not reset shallow url updates on prefetch', async () => { + const browser = await next.browser('/search-params/shallow') + const button = await browser.elementByCss('button') + await button.click() + expect(await browser.url()).toMatch(/\?foo=bar$/) + const link = await browser.elementByCss('a') + await link.hover() + // Hovering a prefetch link should keep the URL intact + expect(await browser.url()).toMatch(/\?foo=bar$/) + }) + describe('useParams identity between renders', () => { async function runTests(page: string) { const browser = await next.browser(page)