app router: Fix infinite redirect loop in MPA navigation

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 <Offscreen>. 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.

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 (
+    <>
+      <button
+        id="go"
+        onClick={() => {
+          // 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) => {
+              let key = `${storageKey}/navigate-${event.destination.url}`
+              let count = +sessionStorage.getItem(key)
+              sessionStorage.setItem(key, count + 1)
+            })
+          }
+
+          startedNavigating = true
+          startTransition(() => {
+            router.push('https://example.vercel.sh/stuff?abc=123')
+          })
+        }}
+      >
+        go
+      </button>
+      <pre id="storage">{JSON.stringify(storage, null, 2)}</pre>
+    </>
+  )
+}

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 <pre id="storage">{JSON.stringify(storage, null, 2)}</pre>
+  } 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/')
+  }
+}

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 <HotReload>?
+            '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')