Added support for query params on not found pages (#43836)

Previously, query parameters were not available on 404 pages because
calling the router methods would change the pathname in the browser.
This change adds support for the query to update for those pages without
updating the path to include the basePath.

This additionally narrows some Typescript types that were previous set
to `any` which highlighted some type errors that were corrected.

Fixes: https://github.com/vercel/next.js/issues/35990

Co-authored-by: JJ Kasper <jj@jjsweb.site>

packages/next/client/index.tsx

@@ -116,11 +116,6 @@ class Container extends React.Component<{
     // - if rewrites in next.config.js match (may have rewrite params)
     if (
       router.isSsr &&
-      // We don't update for 404 requests as this can modify
-      // the asPath unexpectedly e.g. adding basePath when
-      // it wasn't originally present
-      initialData.page !== '/404' &&
-      initialData.page !== '/_error' &&
       (initialData.isFallback ||
         (initialData.nextExport &&
           (isDynamicRoute(router.pathname) ||

packages/next/shared/lib/router/router.ts

@@ -219,11 +219,21 @@ export function interpolateAs(
  * Resolves a given hyperlink with a certain router state (basePath not included).
  * Preserves absolute urls.
  */
+export function resolveHref(
+  router: NextRouter,
+  href: Url,
+  resolveAs: true
+): [string, string] | [string]
+export function resolveHref(
+  router: NextRouter,
+  href: Url,
+  resolveAs?: false
+): string
 export function resolveHref(
   router: NextRouter,
   href: Url,
   resolveAs?: boolean
-): string {
+): [string, string] | [string] | string {
   // we use a dummy base url for relative urls
   let base: URL
   let urlAsString = typeof href === 'string' ? href : formatWithValidation(href)
@@ -293,11 +303,11 @@ export function resolveHref(
         ? finalUrl.href.slice(finalUrl.origin.length)
         : finalUrl.href
 
-    return (
+    return resolveAs
-      resolveAs ? [resolvedHref, interpolatedAs || resolvedHref] : resolvedHref
+      ? [resolvedHref, interpolatedAs || resolvedHref]
-    ) as string
+      : resolvedHref
   } catch (_) {
-    return (resolveAs ? [urlAsString] : urlAsString) as string
+    return resolveAs ? [urlAsString] : urlAsString
   }
 }
 
@@ -306,20 +316,20 @@ function prepareUrlAs(router: NextRouter, url: Url, as?: Url) {
   // we'll format them into the string version here.
   let [resolvedHref, resolvedAs] = resolveHref(router, url, true)
   const origin = getLocationOrigin()
-  const hrefHadOrigin = resolvedHref.startsWith(origin)
+  const hrefWasAbsolute = resolvedHref.startsWith(origin)
-  const asHadOrigin = resolvedAs && resolvedAs.startsWith(origin)
+  const asWasAbsolute = resolvedAs && resolvedAs.startsWith(origin)
 
   resolvedHref = stripOrigin(resolvedHref)
   resolvedAs = resolvedAs ? stripOrigin(resolvedAs) : resolvedAs
 
-  const preparedUrl = hrefHadOrigin ? resolvedHref : addBasePath(resolvedHref)
+  const preparedUrl = hrefWasAbsolute ? resolvedHref : addBasePath(resolvedHref)
   const preparedAs = as
     ? stripOrigin(resolveHref(router, as))
     : resolvedAs || resolvedHref
 
   return {
     url: preparedUrl,
-    as: asHadOrigin ? preparedAs : addBasePath(preparedAs),
+    as: asWasAbsolute ? preparedAs : addBasePath(preparedAs),
   }
 }
 
@@ -488,37 +498,43 @@ function getMiddlewareData<T extends FetchDataOutput>(
   return Promise.resolve({ type: 'next' as const })
 }
 
-function withMiddlewareEffects<T extends FetchDataOutput>(
+interface WithMiddlewareEffectsOutput extends FetchDataOutput {
-  options: MiddlewareEffectParams<T>
+  effect: Awaited<ReturnType<typeof getMiddlewareData>>
-) {
+}
-  return matchesMiddleware(options).then((matches) => {
-    if (matches && options.fetchData) {
-      return options
-        .fetchData()
-        .then((data) =>
-          getMiddlewareData(data.dataHref, data.response, options).then(
-            (effect) => ({
-              dataHref: data.dataHref,
-              cacheKey: data.cacheKey,
-              json: data.json,
-              response: data.response,
-              text: data.text,
-              effect,
-            })
-          )
-        )
-        .catch((_err) => {
-          /**
-           * TODO: Revisit this in the future.
-           * For now we will not consider middleware data errors to be fatal.
-           * maybe we should revisit in the future.
-           */
-          return null
-        })
-    }
 
+async function withMiddlewareEffects<T extends FetchDataOutput>(
+  options: MiddlewareEffectParams<T>
+): Promise<WithMiddlewareEffectsOutput | null> {
+  const matches = await matchesMiddleware(options)
+  if (!matches || !options.fetchData) {
     return null
-  })
+  }
+
+  try {
+    const data = await options.fetchData()
+
+    const effect = await getMiddlewareData(
+      data.dataHref,
+      data.response,
+      options
+    )
+
+    return {
+      dataHref: data.dataHref,
+      json: data.json,
+      response: data.response,
+      text: data.text,
+      cacheKey: data.cacheKey,
+      effect,
+    }
+  } catch {
+    /**
+     * TODO: Revisit this in the future.
+     * For now we will not consider middleware data errors to be fatal.
+     * maybe we should revisit in the future.
+     */
+    return null
+  }
 }
 
 type Url = UrlObject | string
@@ -1208,7 +1224,7 @@ 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.
-    const isQueryUpdating = (options as any)._h
+    const isQueryUpdating = (options as any)._h === 1
     let shouldResolveHref =
       isQueryUpdating ||
       (options as any)._shouldResolveHref ||
@@ -1655,8 +1671,6 @@ export default class Router implements BaseRouter {
         }
       }
 
-      let { error, props, __N_SSG, __N_SSP } = routeInfo
-
       const component: any = routeInfo.Component
       if (component && component.unstable_scriptLoader) {
         const scripts = [].concat(component.unstable_scriptLoader())
@@ -1667,19 +1681,22 @@ export default class Router implements BaseRouter {
       }
 
       // handle redirect on client-transition
-      if ((__N_SSG || __N_SSP) && props) {
+      if ((routeInfo.__N_SSG || routeInfo.__N_SSP) && routeInfo.props) {
-        if (props.pageProps && props.pageProps.__N_REDIRECT) {
+        if (
+          routeInfo.props.pageProps &&
+          routeInfo.props.pageProps.__N_REDIRECT
+        ) {
           // Use the destination from redirect without adding locale
           options.locale = false
 
-          const destination = props.pageProps.__N_REDIRECT
+          const destination = routeInfo.props.pageProps.__N_REDIRECT
 
           // check if destination is internal (resolves to a page) and attempt
           // client-navigation if it is falling back to hard navigation if
           // it's not
           if (
             destination.startsWith('/') &&
-            props.pageProps.__N_REDIRECT_BASE_PATH !== false
+            routeInfo.props.pageProps.__N_REDIRECT_BASE_PATH !== false
           ) {
             const parsedHref = parseRelativeUrl(destination)
             parsedHref.pathname = resolveDynamicRoute(
@@ -1698,10 +1715,10 @@ export default class Router implements BaseRouter {
           return new Promise(() => {})
         }
 
-        nextState.isPreview = !!props.__N_PREVIEW
+        nextState.isPreview = !!routeInfo.props.__N_PREVIEW
 
         // handle SSG data 404
-        if (props.notFound === SSG_DATA_NOT_FOUND) {
+        if (routeInfo.props.notFound === SSG_DATA_NOT_FOUND) {
           let notFoundRoute
 
           try {
@@ -1728,18 +1745,15 @@ export default class Router implements BaseRouter {
         }
       }
 
-      Router.events.emit('beforeHistoryChange', as, routeProps)
-      this.changeState(method, url, as, options)
-
       if (
         isQueryUpdating &&
-        pathname === '/_error' &&
+        this.pathname === '/_error' &&
         self.__NEXT_DATA__.props?.pageProps?.statusCode === 500 &&
-        props?.pageProps
+        routeInfo.props?.pageProps
       ) {
         // ensure statusCode is still correct for static 500 page
         // when updating query information
-        props.pageProps.statusCode = 500
+        routeInfo.props.pageProps.statusCode = 500
       }
 
       // shallow routing is only allowed for same page URL changes.
@@ -1747,8 +1761,9 @@ export default class Router implements BaseRouter {
         options.shallow && nextState.route === (routeInfo.route ?? route)
 
       const shouldScroll =
-        options.scroll ?? (!(options as any)._h && !isValidShallowRoute)
+        options.scroll ?? (!isQueryUpdating && !isValidShallowRoute)
       const resetScroll = shouldScroll ? { x: 0, y: 0 } : null
+      const upcomingScrollState = forcedScroll ?? resetScroll
 
       // the new state that the router gonna set
       const upcomingRouterState = {
@@ -1759,33 +1774,85 @@ export default class Router implements BaseRouter {
         asPath: cleanedAs,
         isFallback: false,
       }
-      const upcomingScrollState = forcedScroll ?? resetScroll
+
+      // When the page being rendered is the 404 page, we should only update the
+      // query parameters. Route changes here might add the basePath when it
+      // wasn't originally present. This is also why this block is before the
+      // below `changeState` call which updates the browser's history (changing
+      // the URL).
+      if (
+        isQueryUpdating &&
+        (this.pathname === '/404' || this.pathname === '/_error')
+      ) {
+        routeInfo = await this.getRouteInfo({
+          route: this.pathname,
+          pathname: this.pathname,
+          query,
+          as,
+          resolvedAs,
+          routeProps: { shallow: false },
+          locale: nextState.locale,
+          isPreview: nextState.isPreview,
+        })
+
+        if ('type' in routeInfo) {
+          throw new Error(`Unexpected middleware effect on ${this.pathname}`)
+        }
+
+        if (
+          this.pathname === '/_error' &&
+          self.__NEXT_DATA__.props?.pageProps?.statusCode === 500 &&
+          routeInfo.props?.pageProps
+        ) {
+          // ensure statusCode is still correct for static 500 page
+          // when updating query information
+          routeInfo.props.pageProps.statusCode = 500
+        }
+
+        try {
+          await this.set(upcomingRouterState, routeInfo, upcomingScrollState)
+        } catch (err) {
+          if (isError(err) && err.cancelled) {
+            Router.events.emit('routeChangeError', err, cleanedAs, routeProps)
+          }
+          throw err
+        }
+
+        return true
+      }
+
+      Router.events.emit('beforeHistoryChange', as, routeProps)
+      this.changeState(method, url, as, options)
 
       // for query updates we can skip it if the state is unchanged and we don't
       // need to scroll
       // https://github.com/vercel/next.js/issues/37139
       const canSkipUpdating =
-        (options as any)._h &&
+        isQueryUpdating &&
         !upcomingScrollState &&
         !readyStateChange &&
         !localeChange &&
         compareRouterStates(upcomingRouterState, this.state)
 
       if (!canSkipUpdating) {
-        await this.set(
+        try {
-          upcomingRouterState,
+          await this.set(upcomingRouterState, routeInfo, upcomingScrollState)
-          routeInfo,
+        } catch (e: any) {
-          upcomingScrollState
+          if (e.cancelled) routeInfo.error = routeInfo.error || e
-        ).catch((e) => {
-          if (e.cancelled) error = error || e
           else throw e
-        })
+        }
 
-        if (error) {
+        if (routeInfo.error) {
           if (!isQueryUpdating) {
-            Router.events.emit('routeChangeError', error, cleanedAs, routeProps)
+            Router.events.emit(
+              'routeChangeError',
+              routeInfo.error,
+              cleanedAs,
+              routeProps
+            )
           }
-          throw error
+
+          throw routeInfo.error
         }
 
         if (process.env.__NEXT_I18N_SUPPORT) {
@@ -1997,9 +2064,13 @@ export default class Router implements BaseRouter {
         isBackground,
       }
 
-      const data =
+      let data:
+        | WithMiddlewareEffectsOutput
+        | (Pick<WithMiddlewareEffectsOutput, 'json'> &
+            Omit<Partial<WithMiddlewareEffectsOutput>, 'json'>)
+        | null =
         isQueryUpdating && !isMiddlewareRewrite
-          ? ({} as any)
+          ? null
           : await withMiddlewareEffects({
               fetchData: () => fetchNextData(fetchNextDataParams),
               asPath: resolvedAs,
@@ -2011,14 +2082,19 @@ export default class Router implements BaseRouter {
               // unless it is a fallback route and the props can't
               // be loaded
               if (isQueryUpdating) {
-                return {} as any
+                return null
               }
               throw err
             })
 
       if (isQueryUpdating) {
-        data.json = self.__NEXT_DATA__.props
+        if (!data) {
+          data = { json: self.__NEXT_DATA__.props }
+        } else {
+          data.json = self.__NEXT_DATA__.props
+        }
       }
+
       handleCancelled()
 
       if (
@@ -2091,40 +2167,42 @@ export default class Router implements BaseRouter {
 
       // For non-SSG prefetches that bailed before sending data
       // we clear the cache to fetch full response
-      if (wasBailedPrefetch) {
+      if (wasBailedPrefetch && data?.dataHref) {
-        delete this.sdc[data?.dataHref]
+        delete this.sdc[data.dataHref]
       }
 
       const { props, cacheKey } = await this._getData(async () => {
         if (shouldFetchData) {
-          const { json, cacheKey: _cacheKey } =
+          if (data?.json && !wasBailedPrefetch) {
-            data?.json && !wasBailedPrefetch
+            return { cacheKey: data.cacheKey, props: data.json }
-              ? data
+          }
-              : await fetchNextData({
+
-                  dataHref: data?.json
+          const dataHref = data?.dataHref
-                    ? data?.dataHref
+            ? data.dataHref
-                    : this.pageLoader.getDataHref({
+            : this.pageLoader.getDataHref({
-                        href: formatWithValidation({ pathname, query }),
+                href: formatWithValidation({ pathname, query }),
-                        asPath: resolvedAs,
+                asPath: resolvedAs,
-                        locale,
+                locale,
-                      }),
+              })
-                  isServerRender: this.isSsr,
+
-                  parseJSON: true,
+          const fetched = await fetchNextData({
-                  inflightCache: wasBailedPrefetch ? {} : this.sdc,
+            dataHref,
-                  persistCache: !isPreview,
+            isServerRender: this.isSsr,
-                  isPrefetch: false,
+            parseJSON: true,
-                  unstable_skipClientCache,
+            inflightCache: wasBailedPrefetch ? {} : this.sdc,
-                })
+            persistCache: !isPreview,
+            isPrefetch: false,
+            unstable_skipClientCache,
+          })
 
           return {
-            cacheKey: _cacheKey,
+            cacheKey: fetched.cacheKey,
-            props: json || {},
+            props: fetched.json || {},
           }
         }
 
         return {
           headers: {},
-          cacheKey: '',
           props: await this.getInitialProps(
             routeInfo.Component,
             // we provide AppTree later so this needs to be `any`
@@ -2143,7 +2221,7 @@ export default class Router implements BaseRouter {
       // Only bust the data cache for SSP routes although
       // middleware can skip cache per request with
       // x-middleware-cache: no-cache as well
-      if (routeInfo.__N_SSP && fetchNextDataParams.dataHref) {
+      if (routeInfo.__N_SSP && fetchNextDataParams.dataHref && cacheKey) {
         delete this.sdc[cacheKey]
       }
 
@@ -2368,7 +2446,7 @@ export default class Router implements BaseRouter {
 
     const data =
       process.env.__NEXT_MIDDLEWARE_PREFETCH === 'strict'
-        ? ({} as any)
+        ? null
         : await withMiddlewareEffects({
             fetchData: () =>
               fetchNextData({
@@ -2492,7 +2570,7 @@ export default class Router implements BaseRouter {
   getInitialProps(
     Component: ComponentType,
     ctx: NextPageContext
-  ): Promise<any> {
+  ): Promise<Record<string, any>> {
     const { Component: App } = this.components['/_app']
     const AppTree = this._wrapApp(App as AppComponent)
     ctx.AppTree = AppTree

test/e2e/404-page-router/app/components/debug-error.js

@@ -0,0 +1,35 @@
+import { useRouter } from 'next/router'
+import { useEffect, useState } from 'react'
+import Debug from './debug'
+
+function transform(router) {
+  return {
+    pathname: router.pathname,
+    asPath: router.asPath,
+    query: Object.entries(router.query)
+      .map(([key, value]) => [key, value].join('='))
+      .join('&'),
+    isReady: router.isReady ? 'true' : 'false',
+  }
+}
+
+export default function DebugError({ children }) {
+  const router = useRouter()
+  const [debug, setDebug] = useState({})
+
+  useEffect(() => {
+    setDebug(transform(router))
+  }, [router])
+
+  return (
+    <>
+      <dl>
+        <Debug name="pathname" value={debug.pathname} />
+        <Debug name="asPath" value={debug.asPath} />
+        <Debug name="query" value={debug.query} />
+        <Debug name="isReady" value={debug.isReady} />
+      </dl>
+      {children}
+    </>
+  )
+}

test/e2e/404-page-router/app/components/debug.js

@@ -0,0 +1,8 @@
+export default function Debug({ name, value }) {
+  return (
+    <>
+      <dt>{name}</dt>
+      <dd id={name}>{value}</dd>
+    </>
+  )
+}

test/e2e/404-page-router/app/middleware.js

@@ -0,0 +1,5 @@
+import { NextResponse } from 'next/server'
+
+export function middleware() {
+  return NextResponse.next()
+}

test/e2e/404-page-router/app/pages/404.js

@@ -0,0 +1,5 @@
+import DebugError from '../components/debug-error'
+
+export default function Custom404() {
+  return <DebugError />
+}

test/e2e/404-page-router/app/pages/_error.js

@@ -0,0 +1,12 @@
+import DebugError from '../components/debug-error'
+
+function Error({ statusCode }) {
+  return <DebugError />
+}
+
+Error.getInitialProps = ({ res, err }) => {
+  const statusCode = res ? res.statusCode : err ? err.statusCode : 404
+  return { statusCode }
+}
+
+export default Error

test/e2e/404-page-router/app/pages/error.js

@@ -0,0 +1,7 @@
+export default function Page() {
+  throw new Error('there was an error')
+}
+
+export const getServerSideProps = () => {
+  return { props: {} }
+}

test/e2e/404-page-router/index.test.ts

@@ -0,0 +1,107 @@
+import { createNext, FileRef } from 'e2e-utils'
+import path from 'path'
+import { type NextInstance } from 'test/lib/next-modes/base'
+import webdriver from 'next-webdriver'
+import { type NextConfig } from 'next'
+
+const pathnames = {
+  '/404': ['/not/a/real/page?with=query', '/not/a/real/page'],
+  // Special handling is done for the error cases because these need to be
+  // prefixed with the basePath if it's enabled for that test suite. These also
+  // should only run when the application is tested in production.
+  '/_error': ['/error?with=query', '/error'],
+}
+
+const basePath = '/docs'
+const i18n = { defaultLocale: 'en-ca', locales: ['en-ca', 'en-fr'] }
+
+const table = [
+  { basePath: false, i18n: true, middleware: false },
+  { basePath: true, i18n: false, middleware: false },
+  { basePath: true, i18n: true, middleware: false },
+  { basePath: false, i18n: false, middleware: false },
+  { basePath: false, i18n: false, middleware: true },
+]
+
+describe.each(table)(
+  '404-page-router with basePath of $basePath and i18n of $i18n and middleware $middleware',
+  (options) => {
+    const isDev = (global as any).isNextDev
+
+    let next: NextInstance
+    let nextConfig: NextConfig
+
+    beforeAll(async () => {
+      const files = {
+        pages: new FileRef(path.join(__dirname, 'app/pages')),
+        components: new FileRef(path.join(__dirname, 'app/components')),
+      }
+
+      // Only add in the middleware if we're testing with middleware enabled.
+      if (options.middleware) {
+        files['middleware.js'] = new FileRef(
+          path.join(__dirname, 'app/middleware.js')
+        )
+      }
+
+      nextConfig = {}
+      if (options.basePath) nextConfig.basePath = basePath
+      if (options.i18n) nextConfig.i18n = i18n
+
+      next = await createNext({ files, nextConfig })
+    })
+
+    afterAll(() => next.destroy())
+
+    /**
+     * translate will iterate over the pathnames and generate the test cases
+     * used in the following table test.
+     *
+     * @param pathname key for the pathname to iterate over
+     * @param shouldPrefixPathname true if the url's should be prefixed with the basePath
+     * @returns test cases
+     */
+    function translate(
+      pathname: keyof typeof pathnames,
+      shouldPrefixPathname: boolean = false
+    ): { url: string; pathname: keyof typeof pathnames; asPath: string }[] {
+      return pathnames[pathname].map((asPath) => ({
+        // Prefix the request URL with the basePath if enabled.
+        url: shouldPrefixPathname ? basePath + asPath : asPath,
+        // The pathname is not prefixed with the basePath.
+        pathname,
+        // The asPath is not prefixed with the basePath.
+        asPath,
+      }))
+    }
+
+    // Always include the /404 tests, they'll run the same in development and
+    // production environments.
+    const urls = translate('/404')
+
+    // Only include the /_error tests in production because in development we
+    // have the error overlay.
+    if (!isDev) {
+      urls.push(...translate('/_error', options.basePath))
+    }
+
+    describe.each(urls)('for $url', ({ url, pathname, asPath }) => {
+      it('should have the correct router parameters after it is ready', async () => {
+        const query = url.split('?')[1] ?? ''
+        const browser = await webdriver(next.url, url)
+
+        try {
+          await browser.waitForCondition(
+            'document.getElementById("isReady")?.innerText === "true"'
+          )
+
+          expect(await browser.elementById('pathname').text()).toEqual(pathname)
+          expect(await browser.elementById('asPath').text()).toEqual(asPath)
+          expect(await browser.elementById('query').text()).toEqual(query)
+        } finally {
+          await browser.close()
+        }
+      })
+    })
+  }
+)