From cba7070c0856c770eae685dd3e5a92c8f2e6e366 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 12 Dec 2022 17:10:44 -0700 Subject: [PATCH] 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 --- packages/next/client/index.tsx | 5 - packages/next/shared/lib/router/router.ts | 268 +++++++++++------- .../app/components/debug-error.js | 35 +++ .../404-page-router/app/components/debug.js | 8 + test/e2e/404-page-router/app/middleware.js | 5 + test/e2e/404-page-router/app/pages/404.js | 5 + test/e2e/404-page-router/app/pages/_error.js | 12 + test/e2e/404-page-router/app/pages/error.js | 7 + test/e2e/404-page-router/index.test.ts | 107 +++++++ 9 files changed, 352 insertions(+), 100 deletions(-) create mode 100644 test/e2e/404-page-router/app/components/debug-error.js create mode 100644 test/e2e/404-page-router/app/components/debug.js create mode 100644 test/e2e/404-page-router/app/middleware.js create mode 100644 test/e2e/404-page-router/app/pages/404.js create mode 100644 test/e2e/404-page-router/app/pages/_error.js create mode 100644 test/e2e/404-page-router/app/pages/error.js create mode 100644 test/e2e/404-page-router/index.test.ts diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index 1a84362885aab2e..66bb9a6b88eb9e7 100644 --- a/packages/next/client/index.tsx +++ b/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) || diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index c9c06916706fce6..6f718b7d1d26863 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/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 ( - resolveAs ? [resolvedHref, interpolatedAs || resolvedHref] : resolvedHref - ) as string + return resolveAs + ? [resolvedHref, interpolatedAs || resolvedHref] + : 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 asHadOrigin = resolvedAs && resolvedAs.startsWith(origin) + const hrefWasAbsolute = resolvedHref.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( return Promise.resolve({ type: 'next' as const }) } -function withMiddlewareEffects( +interface WithMiddlewareEffectsOutput extends FetchDataOutput { + effect: Awaited> +} + +async function withMiddlewareEffects( options: MiddlewareEffectParams -) { - 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 - }) - } +): Promise { + 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 (props.pageProps && props.pageProps.__N_REDIRECT) { + if ((routeInfo.__N_SSG || routeInfo.__N_SSP) && routeInfo.props) { + 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( - upcomingRouterState, - routeInfo, - upcomingScrollState - ).catch((e) => { - if (e.cancelled) error = error || e + try { + await this.set(upcomingRouterState, routeInfo, upcomingScrollState) + } catch (e: any) { + if (e.cancelled) routeInfo.error = routeInfo.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 & + Omit, '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) { - delete this.sdc[data?.dataHref] + if (wasBailedPrefetch && data?.dataHref) { + delete this.sdc[data.dataHref] } const { props, cacheKey } = await this._getData(async () => { if (shouldFetchData) { - const { json, cacheKey: _cacheKey } = - data?.json && !wasBailedPrefetch - ? data - : await fetchNextData({ - dataHref: data?.json - ? data?.dataHref - : this.pageLoader.getDataHref({ - href: formatWithValidation({ pathname, query }), - asPath: resolvedAs, - locale, - }), - isServerRender: this.isSsr, - parseJSON: true, - inflightCache: wasBailedPrefetch ? {} : this.sdc, - persistCache: !isPreview, - isPrefetch: false, - unstable_skipClientCache, - }) + if (data?.json && !wasBailedPrefetch) { + return { cacheKey: data.cacheKey, props: data.json } + } + + const dataHref = data?.dataHref + ? data.dataHref + : this.pageLoader.getDataHref({ + href: formatWithValidation({ pathname, query }), + asPath: resolvedAs, + locale, + }) + + const fetched = await fetchNextData({ + dataHref, + isServerRender: this.isSsr, + parseJSON: true, + inflightCache: wasBailedPrefetch ? {} : this.sdc, + persistCache: !isPreview, + isPrefetch: false, + unstable_skipClientCache, + }) return { - cacheKey: _cacheKey, - props: json || {}, + cacheKey: fetched.cacheKey, + 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 { + ): Promise> { const { Component: App } = this.components['/_app'] const AppTree = this._wrapApp(App as AppComponent) ctx.AppTree = AppTree diff --git a/test/e2e/404-page-router/app/components/debug-error.js b/test/e2e/404-page-router/app/components/debug-error.js new file mode 100644 index 000000000000000..b645fcc9578b9a0 --- /dev/null +++ b/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 ( + <> +
+ + + + +
+ {children} + + ) +} diff --git a/test/e2e/404-page-router/app/components/debug.js b/test/e2e/404-page-router/app/components/debug.js new file mode 100644 index 000000000000000..d50895224c110ab --- /dev/null +++ b/test/e2e/404-page-router/app/components/debug.js @@ -0,0 +1,8 @@ +export default function Debug({ name, value }) { + return ( + <> +
{name}
+
{value}
+ + ) +} diff --git a/test/e2e/404-page-router/app/middleware.js b/test/e2e/404-page-router/app/middleware.js new file mode 100644 index 000000000000000..a2089b0349d3fb7 --- /dev/null +++ b/test/e2e/404-page-router/app/middleware.js @@ -0,0 +1,5 @@ +import { NextResponse } from 'next/server' + +export function middleware() { + return NextResponse.next() +} diff --git a/test/e2e/404-page-router/app/pages/404.js b/test/e2e/404-page-router/app/pages/404.js new file mode 100644 index 000000000000000..7ad2df988adf240 --- /dev/null +++ b/test/e2e/404-page-router/app/pages/404.js @@ -0,0 +1,5 @@ +import DebugError from '../components/debug-error' + +export default function Custom404() { + return +} diff --git a/test/e2e/404-page-router/app/pages/_error.js b/test/e2e/404-page-router/app/pages/_error.js new file mode 100644 index 000000000000000..ec1b38ba55786c7 --- /dev/null +++ b/test/e2e/404-page-router/app/pages/_error.js @@ -0,0 +1,12 @@ +import DebugError from '../components/debug-error' + +function Error({ statusCode }) { + return +} + +Error.getInitialProps = ({ res, err }) => { + const statusCode = res ? res.statusCode : err ? err.statusCode : 404 + return { statusCode } +} + +export default Error diff --git a/test/e2e/404-page-router/app/pages/error.js b/test/e2e/404-page-router/app/pages/error.js new file mode 100644 index 000000000000000..7e1d73161693037 --- /dev/null +++ b/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: {} } +} diff --git a/test/e2e/404-page-router/index.test.ts b/test/e2e/404-page-router/index.test.ts new file mode 100644 index 000000000000000..0935849ab34c0bb --- /dev/null +++ b/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() + } + }) + }) + } +)