Fix basepath router events (#14848)

Co-authored-by: NJoe Haddad <joe.haddad@zeit.co>

docs/api-reference/next/router.md

@@ -318,6 +318,8 @@ You can listen to different events happening inside the Next.js Router. Here's a
 - `hashChangeComplete(url)` - Fires when the hash has changed but not the page
 
 > Here `url` is the URL shown in the browser. If you call `router.push(url, as)` (or similar), then the value of `url` will be `as`.
+>
+> **Note:** If you [configure a `basePath`](/docs/api-reference/next.config.js/basepath.md) then the value of `url` will be `basePath + as`.
 
 #### Usage
 

packages/next/next-server/lib/router/router.ts

@@ -22,6 +22,8 @@ import {
   normalizePathTrailingSlash,
 } from '../../../client/normalize-trailing-slash'
 
+const ABORTED = Symbol('aborted')
+
 const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || ''
 
 export function addBasePath(path: string): string {
@@ -187,6 +189,7 @@ export default class Router implements BaseRouter {
   _wrapApp: (App: ComponentType) => any
   isSsr: boolean
   isFallback: boolean
+  _inFlightRoute?: string
 
   static events: MittEmitter = mitt()
 
@@ -245,9 +248,7 @@ export default class Router implements BaseRouter {
     // until after mount to prevent hydration mismatch
     this.asPath =
       // @ts-ignore this is temporarily global (attached to window)
-      isDynamicRoute(pathname) && __NEXT_DATA__.autoExport
-        ? pathname
-        : delBasePath(as)
+      isDynamicRoute(pathname) && __NEXT_DATA__.autoExport ? pathname : as
     this.basePath = basePath
     this.sub = subscription
     this.clc = null
@@ -419,13 +420,12 @@ export default class Router implements BaseRouter {
     return this.change('replaceState', url, as, options)
   }
 
-  change(
+  async change(
     method: HistoryMethod,
     url: string,
     as: string,
     options: any
   ): Promise<boolean> {
-    return new Promise((resolve, reject) => {
     if (!options._h) {
       this.isSsr = false
     }
@@ -445,7 +445,12 @@ export default class Router implements BaseRouter {
       }
     }
 
-      this.abortComponentLoad(as)
+    if (this._inFlightRoute) {
+      this.abortComponentLoad(this._inFlightRoute)
+    }
+
+    const cleanedAs = delBasePath(as)
+    this._inFlightRoute = as
 
     // If the url change is only related to a hash change
     // We should not proceed. We should only change the state.
@@ -453,18 +458,18 @@ export default class Router implements BaseRouter {
     // WARNING: `_h` is an internal option for handing Next.js client-side
     // hydration. Your app should _never_ use this property. It may change at
     // any time without notice.
-      if (!options._h && this.onlyAHashChange(as)) {
-        this.asPath = as
+    if (!options._h && this.onlyAHashChange(cleanedAs)) {
+      this.asPath = cleanedAs
       Router.events.emit('hashChangeStart', as)
       this.changeState(method, url, as, options)
-        this.scrollToHash(as)
+      this.scrollToHash(cleanedAs)
       Router.events.emit('hashChangeComplete', as)
-        return resolve(true)
+      return true
     }
 
     const parsed = tryParseRelativeUrl(url)
 
-      if (!parsed) return resolve(false)
+    if (!parsed) return false
 
     let { pathname, searchParams } = parsed
     const query = searchParamsToUrlQuery(searchParams)
@@ -476,8 +481,6 @@ export default class Router implements BaseRouter {
       ? removePathTrailingSlash(delBasePath(pathname))
       : pathname
 
-      const cleanedAs = delBasePath(as)
-
     // If asked to change the current URL we should reload the current page
     // (not location.reload() but reload getInitialProps and other Next.js stuffs)
     // We also need to set the method = replaceState always
@@ -509,12 +512,10 @@ export default class Router implements BaseRouter {
             )
           }
 
-            return reject(
-              new Error(
+          throw new Error(
             `The provided \`as\` value (${asPathname}) is incompatible with the \`href\` value (${route}). ` +
               `Read more: https://err.sh/vercel/next.js/incompatible-href-as`
           )
-            )
         }
       } else {
         // Merge params into `query`, overwriting any specified in search
@@ -525,12 +526,18 @@ export default class Router implements BaseRouter {
     Router.events.emit('routeChangeStart', as)
 
     // If shallow is true and the route exists in the router cache we reuse the previous result
-      this.getRouteInfo(route, pathname, query, as, shallow).then(
+    return this.getRouteInfo(route, pathname, query, as, shallow).then(
       (routeInfo) => {
         const { error } = routeInfo
 
         if (error && error.cancelled) {
-            return resolve(false)
+          // An event already has been fired
+          return false
+        }
+
+        if (error && error[ABORTED]) {
+          Router.events.emit('routeChangeError', error, as)
+          return false
         }
 
         Router.events.emit('beforeHistoryChange', as)
@@ -543,9 +550,10 @@ export default class Router implements BaseRouter {
             !(routeInfo.Component as any).getInitialProps
         }
 
-          this.set(route, pathname!, query, cleanedAs, routeInfo).then(() => {
+        return this.set(route, pathname!, query, cleanedAs, routeInfo).then(
+          () => {
             if (error) {
-              Router.events.emit('routeChangeError', error, as)
+              Router.events.emit('routeChangeError', error, cleanedAs)
               throw error
             }
 
@@ -556,12 +564,11 @@ export default class Router implements BaseRouter {
             }
             Router.events.emit('routeChangeComplete', as)
 
-            return resolve(true)
-          })
-        },
-        reject
+            return true
+          }
+        )
+      }
     )
-    })
   }
 
   changeState(
@@ -614,7 +621,7 @@ export default class Router implements BaseRouter {
     }
 
     const handleError = (
-      err: Error & { code: any; cancelled: boolean },
+      err: Error & { code: any; cancelled: boolean; [ABORTED]: boolean },
       loadErrorFail?: boolean
     ) => {
       return new Promise((resolve) => {
@@ -628,8 +635,8 @@ export default class Router implements BaseRouter {
           window.location.href = as
 
           // Changing the URL doesn't block executing the current code path.
-          // So, we need to mark it as a cancelled error and stop the routing logic.
-          err.cancelled = true
+          // So, we need to mark it as aborted and stop the routing logic.
+          err[ABORTED] = true
           // @ts-ignore TODO: fix the control flow here
           return resolve({ error: err })
         }

test/integration/basepath/pages/_app.js

+import { useEffect } from 'react'
+import { useRouter } from 'next/router'
+
+// We use session storage for the event log so that it will survive
+// page reloads, which happen for instance during routeChangeError
+const EVENT_LOG_KEY = 'router-event-log'
+
+function getEventLog() {
+  const data = sessionStorage.getItem(EVENT_LOG_KEY)
+  return data ? JSON.parse(data) : []
+}
+
+function clearEventLog() {
+  sessionStorage.removeItem(EVENT_LOG_KEY)
+}
+
+function addEvent(data) {
+  const eventLog = getEventLog()
+  eventLog.push(data)
+  sessionStorage.setItem(EVENT_LOG_KEY, JSON.stringify(eventLog))
+}
+
+if (typeof window !== 'undefined') {
+  // global functions introduced to interface with the test infrastructure
+  window._clearEventLog = clearEventLog
+  window._getEventLog = getEventLog
+}
+
+function useLoggedEvent(event, serializeArgs = (...args) => args) {
+  const router = useRouter()
+  useEffect(() => {
+    const logEvent = (...args) => {
+      addEvent([event, ...serializeArgs(...args)])
+    }
+    router.events.on(event, logEvent)
+    return () => router.events.off(event, logEvent)
+  }, [event, router.events, serializeArgs])
+}
+
+function serializeErrorEventArgs(err, url) {
+  return [err.message, err.cancelled, url]
+}
+
+export default function MyApp({ Component, pageProps }) {
+  useLoggedEvent('routeChangeStart')
+  useLoggedEvent('routeChangeComplete')
+  useLoggedEvent('routeChangeError', serializeErrorEventArgs)
+  useLoggedEvent('beforeHistoryChange')
+  useLoggedEvent('hashChangeStart')
+  useLoggedEvent('hashChangeComplete')
+  return <Component {...pageProps} />
+}

test/integration/basepath/pages/error-route.js

+export async function getServerSideProps() {
+  // We will use this route to simulate a route change errors
+  throw new Error('KABOOM!')
+}
+
+export default function Page() {
+  return null
+}

test/integration/basepath/pages/hello.js

@@ -58,6 +58,23 @@ export default () => (
     >
       click me for error
     </div>
+    <br />
+    <div id="as-path">{useRouter().asPath}</div>
+    <Link href="/slow-route">
+      <a id="slow-route">
+        <h1>Slow route</h1>
+      </a>
+    </Link>
+    <Link href="/error-route">
+      <a id="error-route">
+        <h1>Error route</h1>
+      </a>
+    </Link>
+    <Link href="/hello#some-hash">
+      <a id="hash-change">
+        <h1>Hash change</h1>
+      </a>
+    </Link>
     <Link href="/something-else" as="/hello">
       <a id="something-else-link">to something else</a>
     </Link>

test/integration/basepath/pages/index.js

@@ -11,7 +11,7 @@ export const getStaticProps = () => {
 }
 
 export default function Index({ hello, nested }) {
-  const { query, pathname } = useRouter()
+  const { query, pathname, asPath } = useRouter()
   return (
     <>
       <h1 id="index-page">index page</h1>
@@ -19,6 +19,7 @@ export default function Index({ hello, nested }) {
       <p id="prop">{hello} world</p>
       <p id="query">{JSON.stringify(query)}</p>
       <p id="pathname">{pathname}</p>
+      <p id="as-path">{asPath}</p>
       <Link href="/hello">
         <a id="hello-link">to /hello</a>
       </Link>

test/integration/basepath/pages/slow-route.js

+export async function getServerSideProps() {
+  // We will use this route to simulate a route cancellation error
+  // by clicking its link twice in rapid succession
+  await new Promise((resolve) => setTimeout(resolve, 5000))
+  return { props: {} }
+}
+
+export default function Page() {
+  return null
+}

test/integration/basepath/test/index.test.js

@@ -406,6 +406,24 @@ const runTests = (context, dev = false) => {
     }
   })
 
+  it('should have correct router paths on first load of /', async () => {
+    const browser = await webdriver(context.appPort, '/docs')
+    await browser.waitForElementByCss('#pathname')
+    const pathname = await browser.elementByCss('#pathname').text()
+    expect(pathname).toBe('/')
+    const asPath = await browser.elementByCss('#as-path').text()
+    expect(asPath).toBe('/')
+  })
+
+  it('should have correct router paths on first load of /hello', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    await browser.waitForElementByCss('#pathname')
+    const pathname = await browser.elementByCss('#pathname').text()
+    expect(pathname).toBe('/hello')
+    const asPath = await browser.elementByCss('#as-path').text()
+    expect(asPath).toBe('/hello')
+  })
+
   it('should fetch data for getStaticProps without reloading', async () => {
     const browser = await webdriver(context.appPort, '/docs/hello')
     await browser.eval('window.beforeNavigate = true')
@@ -490,6 +508,89 @@ const runTests = (context, dev = false) => {
     }
   })
 
+  it('should use urls with basepath in router events', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    try {
+      await browser.eval('window._clearEventLog()')
+      await browser
+        .elementByCss('#other-page-link')
+        .click()
+        .waitForElementByCss('#other-page-title')
+
+      const eventLog = await browser.eval('window._getEventLog()')
+      expect(eventLog).toEqual([
+        ['routeChangeStart', '/docs/other-page'],
+        ['beforeHistoryChange', '/docs/other-page'],
+        ['routeChangeComplete', '/docs/other-page'],
+      ])
+    } finally {
+      await browser.close()
+    }
+  })
+
+  it('should use urls with basepath in router events for hash changes', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    try {
+      await browser.eval('window._clearEventLog()')
+      await browser.elementByCss('#hash-change').click()
+
+      const eventLog = await browser.eval('window._getEventLog()')
+      expect(eventLog).toEqual([
+        ['hashChangeStart', '/docs/hello#some-hash'],
+        ['hashChangeComplete', '/docs/hello#some-hash'],
+      ])
+    } finally {
+      await browser.close()
+    }
+  })
+
+  it('should use urls with basepath in router events for cancelled routes', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    try {
+      await browser.eval('window._clearEventLog()')
+      await browser
+        .elementByCss('#slow-route')
+        .click()
+        .elementByCss('#other-page-link')
+        .click()
+        .waitForElementByCss('#other-page-title')
+
+      const eventLog = await browser.eval('window._getEventLog()')
+      expect(eventLog).toEqual([
+        ['routeChangeStart', '/docs/slow-route'],
+        ['routeChangeError', 'Route Cancelled', true, '/docs/slow-route'],
+        ['routeChangeStart', '/docs/other-page'],
+        ['beforeHistoryChange', '/docs/other-page'],
+        ['routeChangeComplete', '/docs/other-page'],
+      ])
+    } finally {
+      await browser.close()
+    }
+  })
+
+  it('should use urls with basepath in router events for failed route change', async () => {
+    const browser = await webdriver(context.appPort, '/docs/hello')
+    try {
+      await browser.eval('window._clearEventLog()')
+      await browser.elementByCss('#error-route').click()
+
+      await waitFor(2000)
+
+      const eventLog = await browser.eval('window._getEventLog()')
+      expect(eventLog).toEqual([
+        ['routeChangeStart', '/docs/error-route'],
+        [
+          'routeChangeError',
+          'Failed to load static props',
+          null,
+          '/docs/error-route',
+        ],
+      ])
+    } finally {
+      await browser.close()
+    }
+  })
+
   it('should allow URL query strings without refresh', async () => {
     const browser = await webdriver(context.appPort, '/docs/hello?query=true')
     try {

test/integration/dynamic-routing/test/index.test.js

@@ -98,8 +98,11 @@ function runTests(dev) {
 
   it('should allow calling Router.push on mount successfully', async () => {
     const browser = await webdriver(appPort, '/post-1/on-mount-redir')
-    waitFor(2000)
-    expect(await browser.elementByCss('h3').text()).toBe('My blog')
+    try {
+      expect(await browser.waitForElementByCss('h3').text()).toBe('My blog')
+    } finally {
+      browser.close()
+    }
   })
 
   it('should navigate optional dynamic page', async () => {