Skip to content

Commit

Permalink
fix: added support for query params on 404 page
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
wyattjoh committed Dec 7, 2022
1 parent 57515a7 commit 03f7101
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 17 deletions.
4 changes: 0 additions & 4 deletions packages/next/client/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
58 changes: 45 additions & 13 deletions packages/next/shared/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
}

Expand All @@ -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),
}
}

Expand Down Expand Up @@ -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' &&
Expand All @@ -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 = {
Expand All @@ -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
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/404-page-router/app/middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { NextResponse } from 'next/server'

export function middleware() {
return NextResponse.next()
}
40 changes: 40 additions & 0 deletions test/e2e/404-page-router/app/pages/404.js
Original file line number Diff line number Diff line change
@@ -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 (
<>
<dt>{name}</dt>
<dd id={name}>{value}</dd>
</>
)
}

export default function Custom404() {
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>
)
}
68 changes: 68 additions & 0 deletions test/e2e/404-page-router/index.test.ts
Original file line number Diff line number Diff line change
@@ -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()
}
})
}
)
}
)

0 comments on commit 03f7101

Please sign in to comment.