From e9aeb9e1b9de8862ea83b55d0ba68575942352e7 Mon Sep 17 00:00:00 2001
From: Zack Tanner <1939140+ztanner@users.noreply.github.com>
Date: Tue, 16 Apr 2024 16:23:39 -0700
Subject: [PATCH] memoize layout router context (#64575)

Since `AppRouterState` is promise-based (so we can `use` the values and
suspend in render), the state itself isn't stable between renders. Each
action corresponds with a new Promise object. When a link is hovered, a
prefetch action is dispatched, even if the prefetch never happens (for
example, if there's already a prefetch entry in the cache, and it
doesn't need to prefetch again). In other words, the prefetch action
will be dispatched but won't necessarily change the state.

This means that these no-op actions that don't actually change the state
values will trigger a re-render. Most of the context providers in
`AppRouter` are memoized with the exception of `LayoutRouter` context.
This PR memoizes those values so that consumers are only notified of
meaningful updates.

Fixes #63159


Closes NEXT-3127
---
 .../next/src/client/components/app-router.tsx | 40 +++++++++++--------
 .../app-prefetch/app/with-error/error.tsx     | 23 +++++++++++
 .../app-prefetch/app/with-error/page.tsx      | 16 ++++++++
 .../app-dir/app-prefetch/prefetching.test.ts  | 17 ++++++++
 4 files changed, 79 insertions(+), 17 deletions(-)
 create mode 100644 test/e2e/app-dir/app-prefetch/app/with-error/error.tsx
 create mode 100644 test/e2e/app-dir/app-prefetch/app/with-error/page.tsx

diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx
index 86ea7dfd478eb..99b3b8660be5d 100644
--- a/packages/next/src/client/components/app-router.tsx
+++ b/packages/next/src/client/components/app-router.tsx
@@ -618,6 +618,27 @@ function Router({
     return getSelectedParams(tree)
   }, [tree])
 
+  const layoutRouterContext = useMemo(() => {
+    return {
+      childNodes: cache.parallelRoutes,
+      tree,
+      // Root node always has `url`
+      // Provided in AppTreeContext to ensure it can be overwritten in layout-router
+      url: canonicalUrl,
+      loading: cache.loading,
+    }
+  }, [cache.parallelRoutes, tree, canonicalUrl, cache.loading])
+
+  const globalLayoutRouterContext = useMemo(() => {
+    return {
+      buildId,
+      changeByServerResponse,
+      tree,
+      focusAndScrollRef,
+      nextUrl,
+    }
+  }, [buildId, changeByServerResponse, tree, focusAndScrollRef, nextUrl])
+
   let head
   if (matchingHead !== null) {
     // The head is wrapped in an extra component so we can use
@@ -668,25 +689,10 @@ function Router({
         <PathnameContext.Provider value={pathname}>
           <SearchParamsContext.Provider value={searchParams}>
             <GlobalLayoutRouterContext.Provider
-              value={{
-                buildId,
-                changeByServerResponse,
-                tree,
-                focusAndScrollRef,
-                nextUrl,
-              }}
+              value={globalLayoutRouterContext}
             >
               <AppRouterContext.Provider value={appRouter}>
-                <LayoutRouterContext.Provider
-                  value={{
-                    childNodes: cache.parallelRoutes,
-                    tree,
-                    // Root node always has `url`
-                    // Provided in AppTreeContext to ensure it can be overwritten in layout-router
-                    url: canonicalUrl,
-                    loading: cache.loading,
-                  }}
-                >
+                <LayoutRouterContext.Provider value={layoutRouterContext}>
                   {content}
                 </LayoutRouterContext.Provider>
               </AppRouterContext.Provider>
diff --git a/test/e2e/app-dir/app-prefetch/app/with-error/error.tsx b/test/e2e/app-dir/app-prefetch/app/with-error/error.tsx
new file mode 100644
index 0000000000000..2b29ccbc52cab
--- /dev/null
+++ b/test/e2e/app-dir/app-prefetch/app/with-error/error.tsx
@@ -0,0 +1,23 @@
+'use client'
+
+export default function Error({
+  error,
+  reset,
+}: {
+  error: Error & {
+    digest?: string
+    statusCode: number
+  }
+  reset: () => void
+}) {
+  console.log('error render')
+
+  return (
+    <main>
+      <h1>Error</h1>
+      <div id="random-number">Random: {Math.random()}</div>
+      <p>{error.message}</p>
+      <button onClick={reset}>Reset</button>
+    </main>
+  )
+}
diff --git a/test/e2e/app-dir/app-prefetch/app/with-error/page.tsx b/test/e2e/app-dir/app-prefetch/app/with-error/page.tsx
new file mode 100644
index 0000000000000..a12e3d7123d5f
--- /dev/null
+++ b/test/e2e/app-dir/app-prefetch/app/with-error/page.tsx
@@ -0,0 +1,16 @@
+'use client'
+
+import { useState } from 'react'
+
+/** Add your relevant code here for the issue to reproduce */
+export default function Page() {
+  const [error, setError] = useState(false)
+  if (error) {
+    throw new Error('This is a test error')
+  }
+  return (
+    <>
+      <button onClick={() => setError(true)}>Throw Error</button>
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/app-prefetch/prefetching.test.ts b/test/e2e/app-dir/app-prefetch/prefetching.test.ts
index 50b83fee6867f..1ba21a5d6796c 100644
--- a/test/e2e/app-dir/app-prefetch/prefetching.test.ts
+++ b/test/e2e/app-dir/app-prefetch/prefetching.test.ts
@@ -301,6 +301,23 @@ createNextDescribe(
       expect(prefetchResponse).toContain('Loading Prefetch Auto')
     })
 
+    it('should not re-render error component when triggering a prefetch action', async () => {
+      const browser = await next.browser('/with-error')
+
+      const initialRandom = await browser
+        .elementByCss('button')
+        .click()
+        .waitForElementByCss('#random-number')
+        .text()
+
+      await browser.eval('window.next.router.prefetch("/")')
+
+      // confirm the error component was not re-rendered
+      expect(await browser.elementById('random-number').text()).toBe(
+        initialRandom
+      )
+    })
+
     describe('dynamic rendering', () => {
       describe.each(['/force-dynamic', '/revalidate-0'])('%s', (basePath) => {
         it('should not re-render layout when navigating between sub-pages', async () => {