From 7dd61b47a23fcf00fb710db0a4c8488cea89cb01 Mon Sep 17 00:00:00 2001 From: Jan Potoms <2109932+Janpot@users.noreply.github.com> Date: Mon, 20 Jul 2020 22:03:49 +0200 Subject: [PATCH] Fix basepath router events (#14848) Co-authored-by: Joe Haddad --- docs/api-reference/next/router.md | 2 + .../next/next-server/lib/router/router.ts | 229 +++++++++--------- test/integration/basepath/pages/_app.js | 52 ++++ .../integration/basepath/pages/error-route.js | 8 + test/integration/basepath/pages/hello.js | 17 ++ test/integration/basepath/pages/index.js | 3 +- test/integration/basepath/pages/slow-route.js | 10 + test/integration/basepath/test/index.test.js | 101 ++++++++ .../dynamic-routing/test/index.test.js | 7 +- 9 files changed, 315 insertions(+), 114 deletions(-) create mode 100644 test/integration/basepath/pages/_app.js create mode 100644 test/integration/basepath/pages/error-route.js create mode 100644 test/integration/basepath/pages/slow-route.js diff --git a/docs/api-reference/next/router.md b/docs/api-reference/next/router.md index cd474b4112..db22d750d5 100644 --- a/docs/api-reference/next/router.md +++ b/docs/api-reference/next/router.md @@ -318,6 +318,8 @@ You can listen to different events happening inside the Next.js Router. Here's a - `hashChangeComplete(url)` - Fires when the hash has changed but not the page > Here `url` is the URL shown in the browser. If you call `router.push(url, as)` (or similar), then the value of `url` will be `as`. +> +> **Note:** If you [configure a `basePath`](/docs/api-reference/next.config.js/basepath.md) then the value of `url` will be `basePath + as`. #### Usage diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 9b10e5db6d..3a8fc341c6 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -22,6 +22,8 @@ import { normalizePathTrailingSlash, } from '../../../client/normalize-trailing-slash' +const ABORTED = Symbol('aborted') + const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || '' export function addBasePath(path: string): string { @@ -187,6 +189,7 @@ export default class Router implements BaseRouter { _wrapApp: (App: ComponentType) => any isSsr: boolean isFallback: boolean + _inFlightRoute?: string static events: MittEmitter = mitt() @@ -245,9 +248,7 @@ export default class Router implements BaseRouter { // until after mount to prevent hydration mismatch this.asPath = // @ts-ignore this is temporarily global (attached to window) - isDynamicRoute(pathname) && __NEXT_DATA__.autoExport - ? pathname - : delBasePath(as) + isDynamicRoute(pathname) && __NEXT_DATA__.autoExport ? pathname : as this.basePath = basePath this.sub = subscription this.clc = null @@ -419,133 +420,140 @@ export default class Router implements BaseRouter { return this.change('replaceState', url, as, options) } - change( + async change( method: HistoryMethod, url: string, as: string, options: any ): Promise { - return new Promise((resolve, reject) => { - if (!options._h) { - this.isSsr = false - } - // marking route changes as a navigation start entry - if (ST) { - performance.mark('routeChange') - } + if (!options._h) { + this.isSsr = false + } + // marking route changes as a navigation start entry + if (ST) { + performance.mark('routeChange') + } - // Add the ending slash to the paths. So, we can serve the - // "/index.html" directly for the SSR page. - if (process.env.__NEXT_EXPORT_TRAILING_SLASH) { - const rewriteUrlForNextExport = require('./rewrite-url-for-export') - .rewriteUrlForNextExport - // @ts-ignore this is temporarily global (attached to window) - if (__NEXT_DATA__.nextExport) { - as = rewriteUrlForNextExport(as) - } + // Add the ending slash to the paths. So, we can serve the + // "/index.html" directly for the SSR page. + if (process.env.__NEXT_EXPORT_TRAILING_SLASH) { + const rewriteUrlForNextExport = require('./rewrite-url-for-export') + .rewriteUrlForNextExport + // @ts-ignore this is temporarily global (attached to window) + if (__NEXT_DATA__.nextExport) { + as = rewriteUrlForNextExport(as) } + } - this.abortComponentLoad(as) - - // If the url change is only related to a hash change - // We should not proceed. We should only change the state. - - // 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. - if (!options._h && this.onlyAHashChange(as)) { - this.asPath = as - Router.events.emit('hashChangeStart', as) - this.changeState(method, url, as, options) - this.scrollToHash(as) - Router.events.emit('hashChangeComplete', as) - return resolve(true) - } + if (this._inFlightRoute) { + this.abortComponentLoad(this._inFlightRoute) + } - const parsed = tryParseRelativeUrl(url) + const cleanedAs = delBasePath(as) + this._inFlightRoute = as + + // If the url change is only related to a hash change + // We should not proceed. We should only change the state. + + // 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. + if (!options._h && this.onlyAHashChange(cleanedAs)) { + this.asPath = cleanedAs + Router.events.emit('hashChangeStart', as) + this.changeState(method, url, as, options) + this.scrollToHash(cleanedAs) + Router.events.emit('hashChangeComplete', as) + return true + } - if (!parsed) return resolve(false) + const parsed = tryParseRelativeUrl(url) - let { pathname, searchParams } = parsed - const query = searchParamsToUrlQuery(searchParams) + if (!parsed) return false - // url and as should always be prefixed with basePath by this - // point by either next/link or router.push/replace so strip the - // basePath from the pathname to match the pages dir 1-to-1 - pathname = pathname - ? removePathTrailingSlash(delBasePath(pathname)) - : pathname + let { pathname, searchParams } = parsed + const query = searchParamsToUrlQuery(searchParams) - const cleanedAs = delBasePath(as) + // url and as should always be prefixed with basePath by this + // point by either next/link or router.push/replace so strip the + // basePath from the pathname to match the pages dir 1-to-1 + pathname = pathname + ? removePathTrailingSlash(delBasePath(pathname)) + : pathname - // If asked to change the current URL we should reload the current page - // (not location.reload() but reload getInitialProps and other Next.js stuffs) - // We also need to set the method = replaceState always - // as this should not go into the history (That's how browsers work) - // We should compare the new asPath to the current asPath, not the url - if (!this.urlIsNew(cleanedAs)) { - method = 'replaceState' - } + // If asked to change the current URL we should reload the current page + // (not location.reload() but reload getInitialProps and other Next.js stuffs) + // We also need to set the method = replaceState always + // as this should not go into the history (That's how browsers work) + // We should compare the new asPath to the current asPath, not the url + if (!this.urlIsNew(cleanedAs)) { + method = 'replaceState' + } - const route = removePathTrailingSlash(pathname) - const { shallow = false } = options - - if (isDynamicRoute(route)) { - const { pathname: asPathname } = parseRelativeUrl(cleanedAs) - const routeRegex = getRouteRegex(route) - const routeMatch = getRouteMatcher(routeRegex)(asPathname) - if (!routeMatch) { - const missingParams = Object.keys(routeRegex.groups).filter( - (param) => !query[param] - ) + const route = removePathTrailingSlash(pathname) + const { shallow = false } = options - if (missingParams.length > 0) { - if (process.env.NODE_ENV !== 'production') { - console.warn( - `Mismatching \`as\` and \`href\` failed to manually provide ` + - `the params: ${missingParams.join( - ', ' - )} in the \`href\`'s \`query\`` - ) - } + if (isDynamicRoute(route)) { + const { pathname: asPathname } = parseRelativeUrl(cleanedAs) + const routeRegex = getRouteRegex(route) + const routeMatch = getRouteMatcher(routeRegex)(asPathname) + if (!routeMatch) { + const missingParams = Object.keys(routeRegex.groups).filter( + (param) => !query[param] + ) - return reject( - new Error( - `The provided \`as\` value (${asPathname}) is incompatible with the \`href\` value (${route}). ` + - `Read more: https://err.sh/vercel/next.js/incompatible-href-as` - ) + if (missingParams.length > 0) { + if (process.env.NODE_ENV !== 'production') { + console.warn( + `Mismatching \`as\` and \`href\` failed to manually provide ` + + `the params: ${missingParams.join( + ', ' + )} in the \`href\`'s \`query\`` ) } - } else { - // Merge params into `query`, overwriting any specified in search - Object.assign(query, routeMatch) + + throw new Error( + `The provided \`as\` value (${asPathname}) is incompatible with the \`href\` value (${route}). ` + + `Read more: https://err.sh/vercel/next.js/incompatible-href-as` + ) } + } else { + // Merge params into `query`, overwriting any specified in search + Object.assign(query, routeMatch) } + } - Router.events.emit('routeChangeStart', as) + Router.events.emit('routeChangeStart', as) - // If shallow is true and the route exists in the router cache we reuse the previous result - this.getRouteInfo(route, pathname, query, as, shallow).then( - (routeInfo) => { - const { error } = routeInfo + // If shallow is true and the route exists in the router cache we reuse the previous result + return this.getRouteInfo(route, pathname, query, as, shallow).then( + (routeInfo) => { + const { error } = routeInfo - if (error && error.cancelled) { - return resolve(false) - } + if (error && error.cancelled) { + // An event already has been fired + return false + } - Router.events.emit('beforeHistoryChange', as) - this.changeState(method, url, as, options) + if (error && error[ABORTED]) { + Router.events.emit('routeChangeError', error, as) + return false + } - if (process.env.NODE_ENV !== 'production') { - const appComp: any = this.components['/_app'].Component - ;(window as any).next.isPrerendered = - appComp.getInitialProps === appComp.origGetInitialProps && - !(routeInfo.Component as any).getInitialProps - } + Router.events.emit('beforeHistoryChange', as) + this.changeState(method, url, as, options) + + if (process.env.NODE_ENV !== 'production') { + const appComp: any = this.components['/_app'].Component + ;(window as any).next.isPrerendered = + appComp.getInitialProps === appComp.origGetInitialProps && + !(routeInfo.Component as any).getInitialProps + } - this.set(route, pathname!, query, cleanedAs, routeInfo).then(() => { + return this.set(route, pathname!, query, cleanedAs, routeInfo).then( + () => { if (error) { - Router.events.emit('routeChangeError', error, as) + Router.events.emit('routeChangeError', error, cleanedAs) throw error } @@ -556,12 +564,11 @@ export default class Router implements BaseRouter { } Router.events.emit('routeChangeComplete', as) - return resolve(true) - }) - }, - reject - ) - }) + return true + } + ) + } + ) } changeState( @@ -614,7 +621,7 @@ export default class Router implements BaseRouter { } const handleError = ( - err: Error & { code: any; cancelled: boolean }, + err: Error & { code: any; cancelled: boolean; [ABORTED]: boolean }, loadErrorFail?: boolean ) => { return new Promise((resolve) => { @@ -628,8 +635,8 @@ export default class Router implements BaseRouter { window.location.href = as // Changing the URL doesn't block executing the current code path. - // So, we need to mark it as a cancelled error and stop the routing logic. - err.cancelled = true + // So, we need to mark it as aborted and stop the routing logic. + err[ABORTED] = true // @ts-ignore TODO: fix the control flow here return resolve({ error: err }) } diff --git a/test/integration/basepath/pages/_app.js b/test/integration/basepath/pages/_app.js new file mode 100644 index 0000000000..46b7969fa4 --- /dev/null +++ b/test/integration/basepath/pages/_app.js @@ -0,0 +1,52 @@ +import { useEffect } from 'react' +import { useRouter } from 'next/router' + +// We use session storage for the event log so that it will survive +// page reloads, which happen for instance during routeChangeError +const EVENT_LOG_KEY = 'router-event-log' + +function getEventLog() { + const data = sessionStorage.getItem(EVENT_LOG_KEY) + return data ? JSON.parse(data) : [] +} + +function clearEventLog() { + sessionStorage.removeItem(EVENT_LOG_KEY) +} + +function addEvent(data) { + const eventLog = getEventLog() + eventLog.push(data) + sessionStorage.setItem(EVENT_LOG_KEY, JSON.stringify(eventLog)) +} + +if (typeof window !== 'undefined') { + // global functions introduced to interface with the test infrastructure + window._clearEventLog = clearEventLog + window._getEventLog = getEventLog +} + +function useLoggedEvent(event, serializeArgs = (...args) => args) { + const router = useRouter() + useEffect(() => { + const logEvent = (...args) => { + addEvent([event, ...serializeArgs(...args)]) + } + router.events.on(event, logEvent) + return () => router.events.off(event, logEvent) + }, [event, router.events, serializeArgs]) +} + +function serializeErrorEventArgs(err, url) { + return [err.message, err.cancelled, url] +} + +export default function MyApp({ Component, pageProps }) { + useLoggedEvent('routeChangeStart') + useLoggedEvent('routeChangeComplete') + useLoggedEvent('routeChangeError', serializeErrorEventArgs) + useLoggedEvent('beforeHistoryChange') + useLoggedEvent('hashChangeStart') + useLoggedEvent('hashChangeComplete') + return +} diff --git a/test/integration/basepath/pages/error-route.js b/test/integration/basepath/pages/error-route.js new file mode 100644 index 0000000000..4e95173076 --- /dev/null +++ b/test/integration/basepath/pages/error-route.js @@ -0,0 +1,8 @@ +export async function getServerSideProps() { + // We will use this route to simulate a route change errors + throw new Error('KABOOM!') +} + +export default function Page() { + return null +} diff --git a/test/integration/basepath/pages/hello.js b/test/integration/basepath/pages/hello.js index 3e4cfae004..e31e9c20db 100644 --- a/test/integration/basepath/pages/hello.js +++ b/test/integration/basepath/pages/hello.js @@ -58,6 +58,23 @@ export default () => ( > click me for error +
+
{useRouter().asPath}
+ + +

Slow route

+ + + + +

Error route

+ + + + +

Hash change

+ + to something else diff --git a/test/integration/basepath/pages/index.js b/test/integration/basepath/pages/index.js index 84840d1907..c4550e9874 100644 --- a/test/integration/basepath/pages/index.js +++ b/test/integration/basepath/pages/index.js @@ -11,7 +11,7 @@ export const getStaticProps = () => { } export default function Index({ hello, nested }) { - const { query, pathname } = useRouter() + const { query, pathname, asPath } = useRouter() return ( <>

index page

@@ -19,6 +19,7 @@ export default function Index({ hello, nested }) {

{hello} world

{JSON.stringify(query)}

{pathname}

+

{asPath}

to /hello diff --git a/test/integration/basepath/pages/slow-route.js b/test/integration/basepath/pages/slow-route.js new file mode 100644 index 0000000000..6dcc6898c9 --- /dev/null +++ b/test/integration/basepath/pages/slow-route.js @@ -0,0 +1,10 @@ +export async function getServerSideProps() { + // We will use this route to simulate a route cancellation error + // by clicking its link twice in rapid succession + await new Promise((resolve) => setTimeout(resolve, 5000)) + return { props: {} } +} + +export default function Page() { + return null +} diff --git a/test/integration/basepath/test/index.test.js b/test/integration/basepath/test/index.test.js index ee6b1cea59..2bddf06d05 100644 --- a/test/integration/basepath/test/index.test.js +++ b/test/integration/basepath/test/index.test.js @@ -406,6 +406,24 @@ const runTests = (context, dev = false) => { } }) + it('should have correct router paths on first load of /', async () => { + const browser = await webdriver(context.appPort, '/docs') + await browser.waitForElementByCss('#pathname') + const pathname = await browser.elementByCss('#pathname').text() + expect(pathname).toBe('/') + const asPath = await browser.elementByCss('#as-path').text() + expect(asPath).toBe('/') + }) + + it('should have correct router paths on first load of /hello', async () => { + const browser = await webdriver(context.appPort, '/docs/hello') + await browser.waitForElementByCss('#pathname') + const pathname = await browser.elementByCss('#pathname').text() + expect(pathname).toBe('/hello') + const asPath = await browser.elementByCss('#as-path').text() + expect(asPath).toBe('/hello') + }) + it('should fetch data for getStaticProps without reloading', async () => { const browser = await webdriver(context.appPort, '/docs/hello') await browser.eval('window.beforeNavigate = true') @@ -490,6 +508,89 @@ const runTests = (context, dev = false) => { } }) + it('should use urls with basepath in router events', async () => { + const browser = await webdriver(context.appPort, '/docs/hello') + try { + await browser.eval('window._clearEventLog()') + await browser + .elementByCss('#other-page-link') + .click() + .waitForElementByCss('#other-page-title') + + const eventLog = await browser.eval('window._getEventLog()') + expect(eventLog).toEqual([ + ['routeChangeStart', '/docs/other-page'], + ['beforeHistoryChange', '/docs/other-page'], + ['routeChangeComplete', '/docs/other-page'], + ]) + } finally { + await browser.close() + } + }) + + it('should use urls with basepath in router events for hash changes', async () => { + const browser = await webdriver(context.appPort, '/docs/hello') + try { + await browser.eval('window._clearEventLog()') + await browser.elementByCss('#hash-change').click() + + const eventLog = await browser.eval('window._getEventLog()') + expect(eventLog).toEqual([ + ['hashChangeStart', '/docs/hello#some-hash'], + ['hashChangeComplete', '/docs/hello#some-hash'], + ]) + } finally { + await browser.close() + } + }) + + it('should use urls with basepath in router events for cancelled routes', async () => { + const browser = await webdriver(context.appPort, '/docs/hello') + try { + await browser.eval('window._clearEventLog()') + await browser + .elementByCss('#slow-route') + .click() + .elementByCss('#other-page-link') + .click() + .waitForElementByCss('#other-page-title') + + const eventLog = await browser.eval('window._getEventLog()') + expect(eventLog).toEqual([ + ['routeChangeStart', '/docs/slow-route'], + ['routeChangeError', 'Route Cancelled', true, '/docs/slow-route'], + ['routeChangeStart', '/docs/other-page'], + ['beforeHistoryChange', '/docs/other-page'], + ['routeChangeComplete', '/docs/other-page'], + ]) + } finally { + await browser.close() + } + }) + + it('should use urls with basepath in router events for failed route change', async () => { + const browser = await webdriver(context.appPort, '/docs/hello') + try { + await browser.eval('window._clearEventLog()') + await browser.elementByCss('#error-route').click() + + await waitFor(2000) + + const eventLog = await browser.eval('window._getEventLog()') + expect(eventLog).toEqual([ + ['routeChangeStart', '/docs/error-route'], + [ + 'routeChangeError', + 'Failed to load static props', + null, + '/docs/error-route', + ], + ]) + } finally { + await browser.close() + } + }) + it('should allow URL query strings without refresh', async () => { const browser = await webdriver(context.appPort, '/docs/hello?query=true') try { diff --git a/test/integration/dynamic-routing/test/index.test.js b/test/integration/dynamic-routing/test/index.test.js index 59ad2a981a..7e22b5f119 100644 --- a/test/integration/dynamic-routing/test/index.test.js +++ b/test/integration/dynamic-routing/test/index.test.js @@ -98,8 +98,11 @@ function runTests(dev) { it('should allow calling Router.push on mount successfully', async () => { const browser = await webdriver(appPort, '/post-1/on-mount-redir') - waitFor(2000) - expect(await browser.elementByCss('h3').text()).toBe('My blog') + try { + expect(await browser.waitForElementByCss('h3').text()).toBe('My blog') + } finally { + browser.close() + } }) it('should navigate optional dynamic page', async () => { -- GitLab