From 1cec01746cde9d83595b4c0a7639a22dc8d11c6b Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Thu, 9 Jun 2022 16:41:29 +0200 Subject: [PATCH 1/2] Revert "Revert "Avoid unnecessary router state changes" (#37572)" This reverts commit 23295ef34b6105291c3468697a4b21241394827f. --- packages/next/shared/lib/router/router.ts | 96 +++++++++++++------ .../rewrites-client-rerender/next.config.js | 10 ++ .../rewrites-client-rerender/pages/index.js | 13 +++ .../test/index.test.js | 55 +++++++++++ 4 files changed, 146 insertions(+), 28 deletions(-) create mode 100644 test/integration/rewrites-client-rerender/next.config.js create mode 100644 test/integration/rewrites-client-rerender/pages/index.js create mode 100644 test/integration/rewrites-client-rerender/test/index.test.js diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index 17586b1b1f297..90838d4fb5248 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -81,6 +81,37 @@ function buildCancellationError() { }) } +function compareRouterStates(a: Router['state'], b: Router['state']) { + const stateKeys = Object.keys(a) + if (stateKeys.length !== Object.keys(b).length) return false + + for (let i = stateKeys.length; i--; ) { + const key = stateKeys[i] + if (key === 'query') { + const queryKeys = Object.keys(a.query) + if (queryKeys.length !== Object.keys(b.query).length) { + return false + } + for (let j = queryKeys.length; j--; ) { + const queryKey = queryKeys[j] + if ( + !b.query.hasOwnProperty(queryKey) || + a.query[queryKey] !== b.query[queryKey] + ) { + return false + } + } + } else if ( + !b.hasOwnProperty(key) || + a[key as keyof Router['state']] !== b[key as keyof Router['state']] + ) { + return false + } + } + + return true +} + /** * Detects whether a given url is routable by the Next.js router (browser only). */ @@ -1312,38 +1343,47 @@ export default class Router implements BaseRouter { const shouldScroll = options.scroll ?? !isValidShallowRoute const resetScroll = shouldScroll ? { x: 0, y: 0 } : null - await this.set( - { - ...nextState, - route, - pathname, - query, - asPath: cleanedAs, - isFallback: false, - }, - routeInfo, - forcedScroll ?? resetScroll - ).catch((e) => { - if (e.cancelled) error = error || e - else throw e - }) - - if (error) { - Router.events.emit('routeChangeError', error, cleanedAs, routeProps) - throw error + const nextScroll = forcedScroll ?? resetScroll + const mergedNextState = { + ...nextState, + route, + pathname, + query, + asPath: cleanedAs, + isFallback: false, } - if (process.env.__NEXT_I18N_SUPPORT) { - if (nextState.locale) { - document.documentElement.lang = nextState.locale + // 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 && + !nextScroll && + compareRouterStates(mergedNextState, this.state) + + if (!canSkipUpdating) { + await this.set(mergedNextState, routeInfo, nextScroll).catch((e) => { + if (e.cancelled) error = error || e + else throw e + }) + + if (error) { + Router.events.emit('routeChangeError', error, cleanedAs, routeProps) + throw error } - } - Router.events.emit('routeChangeComplete', as, routeProps) - // A hash mark # is the optional last part of a URL - const hashRegex = /#.+$/ - if (shouldScroll && hashRegex.test(as)) { - this.scrollToHash(as) + if (process.env.__NEXT_I18N_SUPPORT) { + if (nextState.locale) { + document.documentElement.lang = nextState.locale + } + } + Router.events.emit('routeChangeComplete', as, routeProps) + + // A hash mark # is the optional last part of a URL + const hashRegex = /#.+$/ + if (shouldScroll && hashRegex.test(as)) { + this.scrollToHash(as) + } } return true diff --git a/test/integration/rewrites-client-rerender/next.config.js b/test/integration/rewrites-client-rerender/next.config.js new file mode 100644 index 0000000000000..111b8e2c548ff --- /dev/null +++ b/test/integration/rewrites-client-rerender/next.config.js @@ -0,0 +1,10 @@ +module.exports = { + rewrites() { + return [ + { + source: '/rewrite', + destination: '/?foo=bar', + }, + ] + }, +} diff --git a/test/integration/rewrites-client-rerender/pages/index.js b/test/integration/rewrites-client-rerender/pages/index.js new file mode 100644 index 0000000000000..59b807ab1e958 --- /dev/null +++ b/test/integration/rewrites-client-rerender/pages/index.js @@ -0,0 +1,13 @@ +import { useEffect } from 'react' +import { useRouter } from 'next/router' + +export default function Index() { + const { query } = useRouter() + + useEffect(() => { + window.__renders = window.__renders || [] + window.__renders.push(query.foo) + }) + + return

A page should not be rerendered if unnecessary.

+} diff --git a/test/integration/rewrites-client-rerender/test/index.test.js b/test/integration/rewrites-client-rerender/test/index.test.js new file mode 100644 index 0000000000000..72412a90aabb6 --- /dev/null +++ b/test/integration/rewrites-client-rerender/test/index.test.js @@ -0,0 +1,55 @@ +/* eslint-env jest */ + +import { join } from 'path' +import { + findPort, + killApp, + launchApp, + nextBuild, + nextStart, +} from 'next-test-utils' +import webdriver from 'next-webdriver' + +const appDir = join(__dirname, '../') + +let appPort +let app + +const runTests = () => { + it('should not trigger unncessary rerenders', async () => { + const browser = await webdriver(appPort, '/') + await new Promise((resolve) => setTimeout(resolve, 100)) + + expect(await browser.eval('window.__renders')).toEqual([undefined]) + }) + + it('should rerender with the correct query parameter if present', async () => { + const browser = await webdriver(appPort, '/rewrite') + await new Promise((resolve) => setTimeout(resolve, 100)) + + expect(await browser.eval('window.__renders')).toEqual([undefined, 'bar']) + }) +} + +describe('rewrites client rerender', () => { + describe('dev mode', () => { + beforeAll(async () => { + appPort = await findPort() + app = await launchApp(appDir, appPort) + }) + afterAll(() => killApp(app)) + + runTests() + }) + + describe('production mode', () => { + beforeAll(async () => { + await nextBuild(appDir) + appPort = await findPort() + app = await nextStart(appDir, appPort) + }) + afterAll(() => killApp(app)) + + runTests() + }) +}) From e883e428c21775c1b9620a070544ff46862e5f7d Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Thu, 9 Jun 2022 17:06:34 +0200 Subject: [PATCH 2/2] make sure to notify the initial load --- packages/next/shared/lib/router/router.ts | 3 +++ test/integration/rewrites-client-rerender/pages/index.js | 6 +++++- .../integration/rewrites-client-rerender/test/index.test.js | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index 90838d4fb5248..2264635cf525f 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -1384,6 +1384,9 @@ export default class Router implements BaseRouter { if (shouldScroll && hashRegex.test(as)) { this.scrollToHash(as) } + } else { + // Still send the event to notify the inital load. + Router.events.emit('routeChangeComplete', as, routeProps) } return true diff --git a/test/integration/rewrites-client-rerender/pages/index.js b/test/integration/rewrites-client-rerender/pages/index.js index 59b807ab1e958..476896e5ba7bf 100644 --- a/test/integration/rewrites-client-rerender/pages/index.js +++ b/test/integration/rewrites-client-rerender/pages/index.js @@ -1,5 +1,9 @@ import { useEffect } from 'react' -import { useRouter } from 'next/router' +import Router, { useRouter } from 'next/router' + +Router.events.on('routeChangeComplete', (url) => { + window.__route_change_complete = url === '/' +}) export default function Index() { const { query } = useRouter() diff --git a/test/integration/rewrites-client-rerender/test/index.test.js b/test/integration/rewrites-client-rerender/test/index.test.js index 72412a90aabb6..f6aa5b4acfa61 100644 --- a/test/integration/rewrites-client-rerender/test/index.test.js +++ b/test/integration/rewrites-client-rerender/test/index.test.js @@ -21,6 +21,7 @@ const runTests = () => { await new Promise((resolve) => setTimeout(resolve, 100)) expect(await browser.eval('window.__renders')).toEqual([undefined]) + expect(await browser.eval('window.__route_change_complete')).toEqual(true) }) it('should rerender with the correct query parameter if present', async () => {