From da56bec3f8afefe64dd76815d0f81771f0a52a65 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 7 Dec 2022 16:32:29 -0700 Subject: [PATCH 1/6] fix: added support for query params on 404 page 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. --- packages/next/client/index.tsx | 4 -- packages/next/shared/lib/router/router.ts | 58 +++++++++++++----- test/e2e/404-page-router/app/middleware.js | 5 ++ test/e2e/404-page-router/app/pages/404.js | 40 +++++++++++++ test/e2e/404-page-router/index.test.ts | 68 ++++++++++++++++++++++ 5 files changed, 158 insertions(+), 17 deletions(-) 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/index.test.ts diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index 1a84362885aab2e..45d9299c8b31b72 100644 --- a/packages/next/client/index.tsx +++ b/packages/next/client/index.tsx @@ -116,10 +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 && diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index c9c06916706fce6..0331c41c9fa1e36 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), } } @@ -1728,9 +1738,6 @@ export default class Router implements BaseRouter { } } - Router.events.emit('beforeHistoryChange', as, routeProps) - this.changeState(method, url, as, options) - if ( isQueryUpdating && pathname === '/_error' && @@ -1749,6 +1756,32 @@ export default class Router implements BaseRouter { const shouldScroll = options.scroll ?? (!(options as any)._h && !isValidShallowRoute) const resetScroll = shouldScroll ? { x: 0, y: 0 } : null + 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') { + try { + await this.set( + { ...nextState, query }, + 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) // the new state that the router gonna set const upcomingRouterState = { @@ -1759,7 +1792,6 @@ export default class Router implements BaseRouter { asPath: cleanedAs, isFallback: false, } - const upcomingScrollState = forcedScroll ?? resetScroll // for query updates we can skip it if the state is unchanged and we don't // need to scroll 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..b82cabc292e26d1 --- /dev/null +++ b/test/e2e/404-page-router/app/pages/404.js @@ -0,0 +1,40 @@ +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}
+ + ) +} + +export default function Custom404() { + const router = useRouter() + const [debug, setDebug] = useState({}) + + useEffect(() => { + setDebug(transform(router)) + }, [router]) + + return ( +
+ + + + +
+ ) +} 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..b7a33c75ae5263f --- /dev/null +++ b/test/e2e/404-page-router/index.test.ts @@ -0,0 +1,68 @@ +import { createNext, FileRef } from 'e2e-utils' +import path from 'path' +import { NextInstance } from 'test/lib/next-modes/base' +import webdriver from 'next-webdriver' + +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 }, +] + +describe.each(table)( + '404-page-router with basePath of $basePath and i18n of $i18n and middleware %middleware', + ({ middleware, ...nextConfig }) => { + let next: NextInstance + + beforeAll(async () => { + const files = { + pages: new FileRef(path.join(__dirname, 'app/pages')), + } + + if (middleware) { + files['middleware.js'] = new FileRef( + path.join(__dirname, 'app/middleware.js') + ) + } + + 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() + } + }) + } + ) + } +) From 09d3db5ff3d55499f70b29df9908ba4752eb86e4 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 8 Dec 2022 16:28:53 -0700 Subject: [PATCH 2/6] fix: fixed tests, support /_error --- packages/next/client/index.tsx | 1 - packages/next/shared/lib/router/router.ts | 157 ++++++++++-------- .../app/components/debug-error.js | 35 ++++ .../404-page-router/app/components/debug.js | 8 + test/e2e/404-page-router/app/pages/404.js | 39 +---- test/e2e/404-page-router/app/pages/_error.js | 13 ++ test/e2e/404-page-router/app/pages/error.js | 7 + test/e2e/404-page-router/index.test.ts | 117 ++++++++----- 8 files changed, 227 insertions(+), 150 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/pages/_error.js create mode 100644 test/e2e/404-page-router/app/pages/error.js diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index 45d9299c8b31b72..66bb9a6b88eb9e7 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 0331c41c9fa1e36..222884d64377120 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 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/pages/404.js b/test/e2e/404-page-router/app/pages/404.js index b82cabc292e26d1..7ad2df988adf240 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 000000000000000..2424b8246f010a6 --- /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 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 index b7a33c75ae5263f..0935849ab34c0bb 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() + } + }) + }) } ) From cd56f0da37a33a915525f3bc8f12e34ae358d80c Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 8 Dec 2022 16:35:48 -0700 Subject: [PATCH 3/6] fix: linting --- test/e2e/404-page-router/app/pages/_error.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/e2e/404-page-router/app/pages/_error.js b/test/e2e/404-page-router/app/pages/_error.js index 2424b8246f010a6..ec1b38ba55786c7 100644 --- a/test/e2e/404-page-router/app/pages/_error.js +++ b/test/e2e/404-page-router/app/pages/_error.js @@ -1,5 +1,4 @@ import DebugError from '../components/debug-error' -import Debug from '../components/debug' function Error({ statusCode }) { return From 059925f62e7c3d2bda4100dda24583545dcd7353 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 12 Dec 2022 11:27:25 -0700 Subject: [PATCH 4/6] fix: handle hydration case for SSR { notFound: true } --- packages/next/shared/lib/router/router.ts | 33 ++++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index 222884d64377120..2db9e6ee1156da7 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -1224,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 || @@ -1746,7 +1746,7 @@ export default class Router implements BaseRouter { if ( isQueryUpdating && - pathname === '/_error' && + this.pathname === '/_error' && self.__NEXT_DATA__.props?.pageProps?.statusCode === 500 && props?.pageProps ) { @@ -1754,13 +1754,12 @@ export default class Router implements BaseRouter { // when updating query information props.pageProps.statusCode = 500 } - // shallow routing is only allowed for same page URL changes. const isValidShallowRoute = 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 @@ -1783,6 +1782,21 @@ export default class Router implements BaseRouter { 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}`) + } + try { await this.set(upcomingRouterState, routeInfo, upcomingScrollState) } catch (err) { @@ -1802,7 +1816,7 @@ export default class Router implements BaseRouter { // need to scroll // https://github.com/vercel/next.js/issues/37139 const canSkipUpdating = - (options as any)._h && + isQueryUpdating && !upcomingScrollState && !readyStateChange && !localeChange && @@ -2058,8 +2072,13 @@ export default class Router implements BaseRouter { }) 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 ( @@ -2411,7 +2430,7 @@ export default class Router implements BaseRouter { const data = process.env.__NEXT_MIDDLEWARE_PREFETCH === 'strict' - ? ({} as any) + ? null : await withMiddlewareEffects({ fetchData: () => fetchNextData({ From cf7c843e1afe79364c29321e125a100c47cace68 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 12 Dec 2022 13:30:11 -0700 Subject: [PATCH 5/6] fix: ensure status code is respected --- packages/next/shared/lib/router/router.ts | 56 +++++++++++++++-------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index 2db9e6ee1156da7..b8a1f3ffd71b184 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -1671,7 +1671,7 @@ export default class Router implements BaseRouter { } } - let { error, props, __N_SSG, __N_SSP } = routeInfo + // let { error, props, __N_SSG, __N_SSP } = routeInfo const component: any = routeInfo.Component if (component && component.unstable_scriptLoader) { @@ -1683,19 +1683,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( @@ -1714,10 +1717,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 { @@ -1748,12 +1751,13 @@ export default class Router implements BaseRouter { isQueryUpdating && 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. const isValidShallowRoute = options.shallow && nextState.route === (routeInfo.route ?? route) @@ -1797,6 +1801,16 @@ export default class Router implements BaseRouter { 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) { @@ -1823,20 +1837,24 @@ export default class Router implements BaseRouter { 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) { From 2466a9021d2b20d25cfeed4e1de31541da937e66 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 12 Dec 2022 13:34:44 -0700 Subject: [PATCH 6/6] fix: linting --- packages/next/shared/lib/router/router.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index b8a1f3ffd71b184..6f718b7d1d26863 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -1671,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())