Revert "Revert "Avoid unnecessary router state changes"" (#37593)

Reverts #37572, with a new test case added about `routeChangeComplete`.

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,50 @@ 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)
+        }
+      } else {
+        // Still send the event to notify the inital load.
+        Router.events.emit('routeChangeComplete', as, routeProps)
       }
 
       return true

test/integration/rewrites-client-rerender/next.config.js

@@ -0,0 +1,10 @@
+module.exports = {
+  rewrites() {
+    return [
+      {
+        source: '/rewrite',
+        destination: '/?foo=bar',
+      },
+    ]
+  },
+}

test/integration/rewrites-client-rerender/pages/index.js

@@ -0,0 +1,17 @@
+import { useEffect } from 'react'
+import Router, { useRouter } from 'next/router'
+
+Router.events.on('routeChangeComplete', (url) => {
+  window.__route_change_complete = url === '/'
+})
+
+export default function Index() {
+  const { query } = useRouter()
+
+  useEffect(() => {
+    window.__renders = window.__renders || []
+    window.__renders.push(query.foo)
+  })
+
+  return <p>A page should not be rerendered if unnecessary.</p>
+}

test/integration/rewrites-client-rerender/test/index.test.js

@@ -0,0 +1,56 @@
+/* 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])
+    expect(await browser.eval('window.__route_change_complete')).toEqual(true)
+  })
+
+  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()
+  })
+})