From 712042f7a90894250acb5a5088e04f92d3662955 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Mon, 1 May 2023 13:31:40 -0700 Subject: [PATCH] app router: Fix infinite redirect loop in MPA navigation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not 100% convinced that this is the correct fix but it's the best I can think of. Previously, we would sometimes call location.assign()/.replace() hundreds of times (or more) as I described in https://github.com/vercel/next.js/issues/48438 and https://github.com/vercel/next.js/issues/48309#issuecomment-1512290958. Sometimes this would just make things slow but the navigation would eventually succeed; sometimes this would just hang the browser. Now we trigger it only once (or—for a reason I don't quite understand—twice in dev, as you can see in the test) and never commit that render. This also fixes the bug I mentioned in https://github.com/vercel/next.js/issues/48438#issuecomment-1528649776 where usePathname() and useSearchParams() would return the page we are navigating to (even if that's an external page wholly unrelated to our site!). Fixes #48309, fixes #48438. --- .../next/src/client/components/app-router.tsx | 36 +++++++--- .../app/external-push/[storageKey]/page.js | 71 +++++++++++++++++++ .../external-log/[storageKey]/page.js | 45 ++++++++++++ .../e2e/app-dir/navigation/navigation.test.ts | 67 ++++++++++++++++- 4 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 test/e2e/app-dir/navigation/app/external-push/[storageKey]/page.js create mode 100644 test/e2e/app-dir/navigation/app/redirect/external-log/[storageKey]/page.js diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index b8a7eed63b79f..b8ad2daea823f 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -1,7 +1,7 @@ 'use client' import type { ReactNode } from 'react' -import React, { useEffect, useMemo, useCallback } from 'react' +import React, { use, useEffect, useMemo, useCallback } from 'react' import { AppRouterContext, LayoutRouterContext, @@ -44,6 +44,7 @@ import { AppRouterAnnouncer } from './app-router-announcer' import { RedirectBoundary } from './redirect-boundary' import { NotFoundBoundary } from './not-found-boundary' import { findHeadInCache } from './router-reducer/reducers/find-head-in-cache' +import { createInfinitePromise } from './infinite-promise' const isServer = typeof window === 'undefined' @@ -85,16 +86,6 @@ function isExternalURL(url: URL) { function HistoryUpdater({ tree, pushRef, canonicalUrl, sync }: any) { // @ts-ignore TODO-APP: useInsertionEffect is available React.useInsertionEffect(() => { - // When mpaNavigation flag is set do a hard navigation to the new url. - if (pushRef.mpaNavigation) { - const location = window.location - if (pushRef.pendingPush) { - location.assign(canonicalUrl) - } else { - location.replace(canonicalUrl) - } - return - } // Identifier is shortened intentionally. // __NA is used to identify if the history entry can be handled by the app-router. // __N is used to identify if the history entry can be handled by the old router. @@ -315,6 +306,29 @@ function Router({ window.nd = { router: appRouter, cache, prefetchCache, tree } } + // When mpaNavigation flag is set do a hard navigation to the new url. + // Infinitely suspend because we don't actually want to rerender any child + // components with the new URL and any entangled state updates shouldn't + // commit either (eg: useTransition isPending should stay true until the page + // unloads). + // + // This is a side effect in render. Don't try this at home, kids. It's + // probably safe because we know this is a singleton component and it's never + // in . At least I hope so. (It will run twice in dev strict mode, + // but that's... fine?) + if (pushRef.mpaNavigation) { + const location = window.location + if (pushRef.pendingPush) { + location.assign(canonicalUrl) + } else { + location.replace(canonicalUrl) + } + // TODO-APP: Should we listen to navigateerror here to catch failed + // navigations somehow? And should we call window.stop() if a SPA navigation + // should interrupt an MPA one? + use(createInfinitePromise()) + } + /** * Handle popstate event, this is used to handle back/forward in the browser. * By default dispatches ACTION_RESTORE, however if the history entry was not pushed/replaced by app-router it will reload the page. diff --git a/test/e2e/app-dir/navigation/app/external-push/[storageKey]/page.js b/test/e2e/app-dir/navigation/app/external-push/[storageKey]/page.js new file mode 100644 index 0000000000000..46db7c95403f0 --- /dev/null +++ b/test/e2e/app-dir/navigation/app/external-push/[storageKey]/page.js @@ -0,0 +1,71 @@ +/*global navigation*/ +'use client' + +import { usePathname, useRouter, useSearchParams } from 'next/navigation' +import { useEffect, useTransition } from 'react' + +let listening = false +let startedNavigating = false + +export default function Page({ params: { storageKey } }) { + if (typeof window === 'undefined') { + throw new Error('Client render only') + } + + let router = useRouter() + let path = usePathname() + let searchParams = useSearchParams().toString() + if (searchParams) { + path += `?${searchParams}` + } + + // Log every pathname+searchParams we've seen + sessionStorage.setItem(`${storageKey}/path-${path}`, 'true') + + let [isPending, startTransition] = useTransition() + useEffect(() => { + if (startedNavigating) { + sessionStorage.setItem(`${storageKey}/lastIsPending`, isPending) + } + }) + + // Read all matching logs and print them + let storage = Object.fromEntries( + Object.entries(sessionStorage).flatMap(([key, value]) => + key.startsWith(`${storageKey}/`) + ? [[key.slice(storageKey.length + 1), value]] + : [] + ) + ) + + return ( + <> + + 
{JSON.stringify(storage, null, 2)}
+ + ) +} diff --git a/test/e2e/app-dir/navigation/app/redirect/external-log/[storageKey]/page.js b/test/e2e/app-dir/navigation/app/redirect/external-log/[storageKey]/page.js new file mode 100644 index 0000000000000..eb0c659890add --- /dev/null +++ b/test/e2e/app-dir/navigation/app/redirect/external-log/[storageKey]/page.js @@ -0,0 +1,45 @@ +/*global navigation*/ +'use client' + +import { redirect, useSearchParams } from 'next/navigation' + +let listening = false + +export default function Page({ params: { storageKey } }) { + if (typeof window === 'undefined') { + throw new Error('Client render only') + } + + let searchParams = useSearchParams() + + if (searchParams.get('read')) { + // Read all matching logs and print them + let storage = Object.fromEntries( + Object.entries(sessionStorage).flatMap(([key, value]) => + key.startsWith(`${storageKey}/`) + ? [[key.slice(storageKey.length + 1), value]] + : [] + ) + ) + + return 
{JSON.stringify(storage, null, 2)}
+ } else { + // Count number of navigations triggered (in browsers that support it) + sessionStorage.setItem( + `${storageKey}/navigation-supported`, + typeof navigation !== 'undefined' + ) + if (!listening && typeof navigation !== 'undefined') { + listening = true + navigation.addEventListener('navigate', (event) => { + if (!event.destination.sameDocument) { + let key = `${storageKey}/navigate-${event.destination.url}` + let count = +sessionStorage.getItem(key) + sessionStorage.setItem(key, count + 1) + } + }) + } + + redirect('https://example.vercel.sh/') + } +} diff --git a/test/e2e/app-dir/navigation/navigation.test.ts b/test/e2e/app-dir/navigation/navigation.test.ts index 5a4dc7e3ec53d..3c5ddc320a77d 100644 --- a/test/e2e/app-dir/navigation/navigation.test.ts +++ b/test/e2e/app-dir/navigation/navigation.test.ts @@ -7,7 +7,7 @@ createNextDescribe( { files: __dirname, }, - ({ next, isNextDeploy }) => { + ({ next, isNextDev, isNextDeploy }) => { describe('query string', () => { it('should set query correctly', async () => { const browser = await webdriver(next.url, '/') @@ -162,6 +162,34 @@ createNextDescribe( 'Example Domain' ) }) + + it('should redirect to external url, initiating only once', async () => { + const storageKey = Math.random() + const browser = await next.browser( + `/redirect/external-log/${storageKey}` + ) + expect(await browser.waitForElementByCss('h1').text()).toBe( + 'Example Domain' + ) + + // Now check the logs... + await browser.get( + `${next.url}/redirect/external-log/${storageKey}?read=1` + ) + const stored = JSON.parse(await browser.elementByCss('pre').text()) + + if (stored['navigation-supported'] === 'false') { + // Old browser. Can't know how many times we navigated. Oh well. + return + } + + expect(stored).toEqual({ + // Not actually sure why this is '2' in dev. Possibly something + // related to an update triggered by ? + 'navigate-https://example.vercel.sh/': isNextDev ? '2' : '1', + 'navigation-supported': 'true', + }) + }) }) describe('next.config.js redirects', () => { @@ -220,6 +248,43 @@ createNextDescribe( }) }) + describe('external push', () => { + it('should push external url without affecting hooks', async () => { + // Log with sessionStorage to persist across navigations + const storageKey = Math.random() + const browser = await next.browser(`/external-push/${storageKey}`) + await browser.elementByCss('#go').click() + await browser.waitForCondition( + 'window.location.origin === "https://example.vercel.sh"' + ) + + // Now check the logs... + await browser.get(`${next.url}/external-push/${storageKey}`) + const stored = JSON.parse(await browser.elementByCss('pre').text()) + let expected = { + // Only one navigation + 'navigate-https://example.vercel.sh/stuff?abc=123': '1', + 'navigation-supported': 'true', + // Make sure /stuff?abc=123 is not logged here + [`path-/external-push/${storageKey}`]: 'true', + // isPending should have been true until the page unloads + lastIsPending: 'true', + } + + if (stored['navigation-supported'] !== 'true') { + // Old browser. Can't know how many times we navigated. Oh well. + expected['navigation-supported'] = 'false' + for (const key in expected) { + if (key.startsWith('navigate-')) { + delete expected[key] + } + } + } + + expect(stored).toEqual(expected) + }) + }) + describe('nested navigation', () => { it('should navigate to nested pages', async () => { const browser = await next.browser('/nested-navigation')