diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index 45d9299c8b31..66bb9a6b88eb 100644 --- a/packages/next/client/index.tsx +++ b/packages/next/client/index.tsx @@ -116,7 +116,6 @@ class Container extends React.Component<{ // - if rewrites in next.config.js match (may have rewrite params) if ( router.isSsr && - 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 99424e410f74..69d40b05f47c 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -498,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 @@ -1758,18 +1764,27 @@ export default class Router implements BaseRouter { const resetScroll = shouldScroll ? { x: 0, y: 0 } : null const upcomingScrollState = forcedScroll ?? resetScroll + // the new state that the router gonna set + const upcomingRouterState = { + ...nextState, + route, + pathname, + query, + asPath: cleanedAs, + isFallback: false, + } + // 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') { + if ( + isQueryUpdating && + (this.pathname === '/404' || this.pathname === '/_error') + ) { try { - await this.set( - { ...nextState, query }, - routeInfo, - upcomingScrollState - ) + await this.set(upcomingRouterState, routeInfo, upcomingScrollState) } catch (err) { if (isError(err) && err.cancelled) { Router.events.emit('routeChangeError', err, cleanedAs, routeProps) @@ -1783,16 +1798,6 @@ export default class Router implements BaseRouter { Router.events.emit('beforeHistoryChange', as, routeProps) this.changeState(method, url, as, options) - // the new state that the router gonna set - const upcomingRouterState = { - ...nextState, - route, - pathname, - query, - asPath: cleanedAs, - isFallback: false, - } - // 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 @@ -2029,9 +2034,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, @@ -2043,13 +2052,13 @@ 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 + data = { json: self.__NEXT_DATA__.props } } handleCancelled() @@ -2123,40 +2132,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` @@ -2175,7 +2186,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] } @@ -2524,7 +2535,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 000000000000..b645fcc9578b --- /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 000000000000..d50895224c11 --- /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/pages/404.js b/test/e2e/404-page-router/app/pages/404.js index b82cabc292e2..7ad2df988adf 100644 --- a/test/e2e/404-page-router/app/pages/404.js +++ b/test/e2e/404-page-router/app/pages/404.js @@ -1,40 +1,5 @@ -import { useRouter } from 'next/router' -import { useEffect, useState } from 'react' - -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', - } -} - -function Debug({ name, value }) { - return ( - <> -
{name}
-
{value}
- - ) -} +import DebugError from '../components/debug-error' export default function Custom404() { - const router = useRouter() - const [debug, setDebug] = useState({}) - - useEffect(() => { - setDebug(transform(router)) - }, [router]) - - return ( -
- - - - -
- ) + 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 000000000000..2424b8246f01 --- /dev/null +++ b/test/e2e/404-page-router/app/pages/_error.js @@ -0,0 +1,13 @@ +import DebugError from '../components/debug-error' +import Debug from '../components/debug' + +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 000000000000..7e1d73161693 --- /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 index b7a33c75ae52..0935849ab34c 100644 --- a/test/e2e/404-page-router/index.test.ts +++ b/test/e2e/404-page-router/index.test.ts @@ -1,68 +1,107 @@ import { createNext, FileRef } from 'e2e-utils' import path from 'path' -import { NextInstance } from 'test/lib/next-modes/base' +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 = [ - // Including the i18n and no basePath - { basePath: undefined, i18n, middleware: false }, - // Including the basePath and no i18n - { basePath, i18n: undefined, middleware: false }, - // Including both i18n and basePath - { basePath, i18n, middleware: false }, - // Including no basePath or i18n - { basePath: undefined, i18n: undefined, middleware: false }, - // Including middleware. - { basePath: undefined, i18n: undefined, middleware: true }, + { 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', - ({ middleware, ...nextConfig }) => { + '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')), } - if (middleware) { + // 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') ) } - next = await createNext({ - files, - nextConfig, - }) + nextConfig = {} + if (options.basePath) nextConfig.basePath = basePath + if (options.i18n) nextConfig.i18n = i18n + + next = await createNext({ files, nextConfig }) }) afterAll(() => next.destroy()) - describe.each(['/not/a/real/page?with=query', '/not/a/real/page'])( - 'for url %s', - (url) => { - 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('/404') - expect(await browser.elementById('asPath').text()).toEqual(url) - expect(await browser.elementById('query').text()).toEqual(query) - } finally { - await browser.close() - } - }) - } - ) + /** + * 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() + } + }) + }) } )