Fix basepath router events

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,133 +420,140 @@ 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
-      }
-      // marking route changes as a navigation start entry
-      if (ST) {
-        performance.mark('routeChange')
-      }
+    if (!options._h) {
+      this.isSsr = false
+    }
+    // marking route changes as a navigation start entry
+    if (ST) {
+      performance.mark('routeChange')
+    }
 
-      // Add the ending slash to the paths. So, we can serve the
-      // "<page>/index.html" directly for the SSR page.
-      if (process.env.__NEXT_EXPORT_TRAILING_SLASH) {
-        const rewriteUrlForNextExport = require('./rewrite-url-for-export')
-          .rewriteUrlForNextExport
-        // @ts-ignore this is temporarily global (attached to window)
-        if (__NEXT_DATA__.nextExport) {
-          as = rewriteUrlForNextExport(as)
-        }
+    // Add the ending slash to the paths. So, we can serve the
+    // "<page>/index.html" directly for the SSR page.
+    if (process.env.__NEXT_EXPORT_TRAILING_SLASH) {
+      const rewriteUrlForNextExport = require('./rewrite-url-for-export')
+        .rewriteUrlForNextExport
+      // @ts-ignore this is temporarily global (attached to window)
+      if (__NEXT_DATA__.nextExport) {
+        as = rewriteUrlForNextExport(as)
       }
+    }
 
-      this.abortComponentLoad(as)
-
-      // If the url change is only related to a hash change
-      // We should not proceed. We should only change the state.
-
-      // 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
-        Router.events.emit('hashChangeStart', as)
-        this.changeState(method, url, as, options)
-        this.scrollToHash(as)
-        Router.events.emit('hashChangeComplete', as)
-        return resolve(true)
-      }
+    if (this._inFlightRoute) {
+      this.abortComponentLoad(this._inFlightRoute)
+    }
 
-      const parsed = tryParseRelativeUrl(url)
+    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.
+
+    // 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(cleanedAs)) {
+      this.asPath = cleanedAs
+      Router.events.emit('hashChangeStart', as)
+      this.changeState(method, url, as, options)
+      this.scrollToHash(cleanedAs)
+      Router.events.emit('hashChangeComplete', as)
+      return true
+    }
 
-      if (!parsed) return resolve(false)
+    const parsed = tryParseRelativeUrl(url)
 
-      let { pathname, searchParams } = parsed
-      const query = searchParamsToUrlQuery(searchParams)
+    if (!parsed) return false
 
-      // url and as should always be prefixed with basePath by this
-      // point by either next/link or router.push/replace so strip the
-      // basePath from the pathname to match the pages dir 1-to-1
-      pathname = pathname
-        ? removePathTrailingSlash(delBasePath(pathname))
-        : pathname
+    let { pathname, searchParams } = parsed
+    const query = searchParamsToUrlQuery(searchParams)
 
-      const cleanedAs = delBasePath(as)
+    // url and as should always be prefixed with basePath by this
+    // point by either next/link or router.push/replace so strip the
+    // basePath from the pathname to match the pages dir 1-to-1
+    pathname = pathname
+      ? removePathTrailingSlash(delBasePath(pathname))
+      : pathname
 
-      // 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
-      // as this should not go into the history (That's how browsers work)
-      // We should compare the new asPath to the current asPath, not the url
-      if (!this.urlIsNew(cleanedAs)) {
-        method = 'replaceState'
-      }
+    // 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
+    // as this should not go into the history (That's how browsers work)
+    // We should compare the new asPath to the current asPath, not the url
+    if (!this.urlIsNew(cleanedAs)) {
+      method = 'replaceState'
+    }
 
-      const route = removePathTrailingSlash(pathname)
-      const { shallow = false } = options
-
-      if (isDynamicRoute(route)) {
-        const { pathname: asPathname } = parseRelativeUrl(cleanedAs)
-        const routeRegex = getRouteRegex(route)
-        const routeMatch = getRouteMatcher(routeRegex)(asPathname)
-        if (!routeMatch) {
-          const missingParams = Object.keys(routeRegex.groups).filter(
-            (param) => !query[param]
-          )
+    const route = removePathTrailingSlash(pathname)
+    const { shallow = false } = options
 
-          if (missingParams.length > 0) {
-            if (process.env.NODE_ENV !== 'production') {
-              console.warn(
-                `Mismatching \`as\` and \`href\` failed to manually provide ` +
-                  `the params: ${missingParams.join(
-                    ', '
-                  )} in the \`href\`'s \`query\``
-              )
-            }
+    if (isDynamicRoute(route)) {
+      const { pathname: asPathname } = parseRelativeUrl(cleanedAs)
+      const routeRegex = getRouteRegex(route)
+      const routeMatch = getRouteMatcher(routeRegex)(asPathname)
+      if (!routeMatch) {
+        const missingParams = Object.keys(routeRegex.groups).filter(
+          (param) => !query[param]
+        )
 
-            return reject(
-              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`
-              )
+        if (missingParams.length > 0) {
+          if (process.env.NODE_ENV !== 'production') {
+            console.warn(
+              `Mismatching \`as\` and \`href\` failed to manually provide ` +
+                `the params: ${missingParams.join(
+                  ', '
+                )} in the \`href\`'s \`query\``
             )
           }
-        } else {
-          // Merge params into `query`, overwriting any specified in search
-          Object.assign(query, routeMatch)
+
+          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
+        Object.assign(query, routeMatch)
       }
+    }
 
-      Router.events.emit('routeChangeStart', as)
+    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(
-        (routeInfo) => {
-          const { error } = routeInfo
+    // If shallow is true and the route exists in the router cache we reuse the previous result
+    return this.getRouteInfo(route, pathname, query, as, shallow).then(
+      (routeInfo) => {
+        const { error } = routeInfo
 
-          if (error && error.cancelled) {
-            return resolve(false)
-          }
+        if (error && error.cancelled) {
+          // An event already has been fired
+          return false
+        }
 
-          Router.events.emit('beforeHistoryChange', as)
-          this.changeState(method, url, as, options)
+        if (error && error[ABORTED]) {
+          Router.events.emit('routeChangeError', error, as)
+          return false
+        }
 
-          if (process.env.NODE_ENV !== 'production') {
-            const appComp: any = this.components['/_app'].Component
-            ;(window as any).next.isPrerendered =
-              appComp.getInitialProps === appComp.origGetInitialProps &&
-              !(routeInfo.Component as any).getInitialProps
-          }
+        Router.events.emit('beforeHistoryChange', as)
+        this.changeState(method, url, as, options)
+
+        if (process.env.NODE_ENV !== 'production') {
+          const appComp: any = this.components['/_app'].Component
+          ;(window as any).next.isPrerendered =
+            appComp.getInitialProps === appComp.origGetInitialProps &&
+            !(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

@@ -0,0 +1,52 @@
+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

@@ -0,0 +1,8 @@
+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
+}