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() + } + }) + }) + } +)