Add state comparison to router (#38422)

Lands #37431 again, but it only solves the re-render issue completely for the middleware case (closes #38267), not the `rewrites` case (#37139).

For `rewrites`, the blocker is `isReady` always being `false` initially and to match the markup on hydration we can't simply work around it without a re-render. Need to come up with another fix.

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm lint`
- [ ] The examples guidelines are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing.md#adding-examples)

packages/next/shared/lib/router/router.ts

@@ -46,6 +46,7 @@ import { addBasePath } from '../../../client/add-base-path'
 import { hasBasePath } from '../../../client/has-base-path'
 import { getNextPathnameInfo } from './utils/get-next-pathname-info'
 import { formatNextPathnameInfo } from './utils/format-next-pathname-info'
+import { compareRouterStates } from './utils/compare-states'
 
 declare global {
   interface Window {
@@ -990,6 +991,7 @@ export default class Router implements BaseRouter {
     // for static pages with query params in the URL we delay
     // marking the router ready until after the query is updated
     // or a navigation has occurred
+    const readyStateChange = this.isReady !== true
     this.isReady = true
     const isSsr = this.isSsr
 
@@ -1127,7 +1129,7 @@ export default class Router implements BaseRouter {
     )
     this._inFlightRoute = as
 
-    let localeChange = prevLocale !== nextState.locale
+    const localeChange = prevLocale !== nextState.locale
 
     // If the url change is only related to a hash change
     // We should not proceed. We should only change the state.
@@ -1481,46 +1483,63 @@ export default class Router implements BaseRouter {
       const isValidShallowRoute =
         options.shallow && nextState.route === (routeInfo.route ?? route)
 
-      const shouldScroll = options.scroll ?? !isValidShallowRoute
+      const shouldScroll =
+        options.scroll ?? (!(options as any)._h && !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
-      })
+      // the new state that the router gonna set
+      const upcomingRouterState = {
+        ...nextState,
+        route,
+        pathname,
+        query,
+        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
+      // https://github.com/vercel/next.js/issues/37139
+      const canSkipUpdating =
+        (options as any)._h &&
+        !upcomingScrollState &&
+        !readyStateChange &&
+        !localeChange &&
+        compareRouterStates(upcomingRouterState, this.state)
+
+      if (!canSkipUpdating) {
+        await this.set(
+          upcomingRouterState,
+          routeInfo,
+          upcomingScrollState
+        ).catch((e) => {
+          if (e.cancelled) error = error || e
+          else throw e
+        })
 
-      if (error) {
-        if (!isQueryUpdating) {
-          Router.events.emit('routeChangeError', error, cleanedAs, routeProps)
+        if (error) {
+          if (!isQueryUpdating) {
+            Router.events.emit('routeChangeError', error, cleanedAs, routeProps)
+          }
+          throw error
         }
-        throw error
-      }
 
-      if (process.env.__NEXT_I18N_SUPPORT) {
-        if (nextState.locale) {
-          document.documentElement.lang = nextState.locale
+        if (process.env.__NEXT_I18N_SUPPORT) {
+          if (nextState.locale) {
+            document.documentElement.lang = nextState.locale
+          }
         }
-      }
 
-      if (!isQueryUpdating) {
-        Router.events.emit('routeChangeComplete', as, routeProps)
-      }
+        if (!isQueryUpdating) {
+          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)
+        // A hash mark # is the optional last part of a URL
+        const hashRegex = /#.+$/
+        if (shouldScroll && hashRegex.test(as)) {
+          this.scrollToHash(as)
+        }
       }
 
       return true

packages/next/shared/lib/router/utils/compare-states.ts

@@ -0,0 +1,32 @@
+import type { default as Router } from '../router'
+
+export 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
+}

test/integration/router-rerender/middleware.js

@@ -0,0 +1,5 @@
+import { NextResponse } from 'next/server'
+
+export function middleware() {
+  return NextResponse.next()
+}

test/integration/router-rerender/next.config.js

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

test/integration/router-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 <p>A page should not be rerendered if unnecessary.</p>
+}

test/integration/router-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 = () => {
+  describe('with middleware', () => {
+    it('should not trigger unncessary rerenders when middleware is used', async () => {
+      const browser = await webdriver(appPort, '/')
+      await new Promise((resolve) => setTimeout(resolve, 100))
+
+      expect(await browser.eval('window.__renders')).toEqual([undefined])
+    })
+  })
+
+  describe('with rewrites', () => {
+    // TODO: Figure out the `isReady` issue.
+    it.skip('should not trigger unncessary rerenders when rewrites are used', async () => {})
+    it.skip('should rerender with the correct query parameter if present with rewrites', async () => {})
+  })
+}
+
+describe('router 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()
+  })
+})