fix erroneous RSC calls on hash changes

vercel · May 31, 2024 · 5744cce · 5744cce
1 parent 1e63f5b
commit 5744cce
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 7 deletions.
diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts
@@ -114,6 +114,8 @@ function navigateReducer_noPPR(
   const mutable: Mutable = {}
   const { hash } = url
   const href = createHrefFromUrl(url)
+  const hrefNoHash = createHrefFromUrl(url, false)
+  const isHashOnlyChange = hrefNoHash === state.nextUrl && !!hash
   const pendingPush = navigateType === 'push'
   // we want to prune the prefetch cache on every navigation to avoid it growing too large
   prunePrefetchCache(state.prefetchCache)
@@ -201,12 +203,15 @@ function navigateReducer_noPPR(
 
           if (
             prefetchValues.status === PrefetchCacheEntryStatus.stale &&
+            !isHashOnlyChange &&
             !isFirstRead
           ) {
             // When we have a stale prefetch entry, we only want to re-use the loading state of the route we're navigating to, to support instant loading navigations
             // this will trigger a lazy fetch for the actual page data by nulling the `rsc` and `prefetchRsc` values for page data,
             // while copying over the `loading` for the segment that contains the page data.
             // We only do this on subsequent reads, as otherwise there'd be no loading data to re-use.
+
+            // We skip this branch if only the hash fragment has changed, as we don't want to trigger a lazy fetch in that case
             applied = triggerLazyFetchForLeafSegments(
               cache,
               currentCache,
@@ -288,7 +293,9 @@ function navigateReducer_PPR(
   const mutable: Mutable = {}
   const { hash } = url
   const href = createHrefFromUrl(url)
+  const hrefNoHash = createHrefFromUrl(url, false)
   const pendingPush = navigateType === 'push'
+  const isHashOnlyChange = hrefNoHash === state.nextUrl && !!hash
   // we want to prune the prefetch cache on every navigation to avoid it growing too large
   prunePrefetchCache(state.prefetchCache)
 
@@ -452,12 +459,15 @@ function navigateReducer_PPR(
 
             if (
               prefetchValues.status === PrefetchCacheEntryStatus.stale &&
+              !isHashOnlyChange &&
               !isFirstRead
             ) {
               // When we have a stale prefetch entry, we only want to re-use the loading state of the route we're navigating to, to support instant loading navigations
               // this will trigger a lazy fetch for the actual page data by nulling the `rsc` and `prefetchRsc` values for page data,
               // while copying over the `loading` for the segment that contains the page data.
               // We only do this on subsequent reads, as otherwise there'd be no loading data to re-use.
+
+              // We skip this branch if only the hash fragment has changed, as we don't want to trigger a lazy fetch in that case
               applied = triggerLazyFetchForLeafSegments(
                 cache,
                 currentCache,

diff --git a/test/e2e/app-dir/navigation/app/hash/page.js b/test/e2e/app-dir/navigation/app/hash/page.js
@@ -29,6 +29,11 @@ export default function HashPage() {
       <Link href="/hash#non-existent" id="link-to-non-existent">
         To non-existent
       </Link>
+      <div>
+        <Link href="?with-query-param#hash-160" id="link-to-query-param">
+          To 160 (with query param)
+        </Link>
+      </div>
       <div>
         {items.map((item) => (
           <div key={item.id}>

diff --git a/test/e2e/app-dir/navigation/navigation.test.ts b/test/e2e/app-dir/navigation/navigation.test.ts
@@ -3,7 +3,7 @@ import { retry, waitFor } from 'next-test-utils'
 import type { Response } from 'playwright'
 
 describe('app dir - navigation', () => {
-  const { next, isNextDev, isNextDeploy } = nextTestSetup({
+  const { next, isNextDev, isNextStart, isNextDeploy } = nextTestSetup({
     files: __dirname,
   })
 
@@ -151,7 +151,17 @@ describe('app dir - navigation', () => {
 
   describe('hash', () => {
     it('should scroll to the specified hash', async () => {
-      const browser = await next.browser('/hash')
+      let hasRscRequest = false
+      const browser = await next.browser('/hash', {
+        beforePageLoad(page) {
+          page.on('request', async (req) => {
+            const headers = await req.allHeaders()
+            if (headers['rsc']) {
+              hasRscRequest = true
+            }
+          })
+        },
+      })
 
       const checkLink = async (
         val: number | string,
@@ -166,13 +176,31 @@ describe('app dir - navigation', () => {
         )
       }
 
-      await checkLink(6, 114)
-      await checkLink(50, 730)
-      await checkLink(160, 2270)
-      await checkLink(300, 4230)
-      await checkLink(500, 7030) // this one is hash only (`href="#hash-500"`)
+      if (isNextStart) {
+        await browser.waitForIdleNetwork()
+        // there should be an RSC call for the prefetch
+        expect(hasRscRequest).toBe(true)
+      }
+
+      // Wait for all network requests to finish, and then initialize the flag
+      // used to determine if any RSC requests are made
+      hasRscRequest = false
+
+      await checkLink(6, 128)
+      await checkLink(50, 744)
+      await checkLink(160, 2284)
+      await checkLink(300, 4244)
+      await checkLink(500, 7044) // this one is hash only (`href="#hash-500"`)
       await checkLink('top', 0)
       await checkLink('non-existent', 0)
+
+      // there should have been no RSC calls to fetch data
+      expect(hasRscRequest).toBe(false)
+
+      // There should be an RSC request if the query param is changed
+      await checkLink('query-param', 2284)
+      await browser.waitForIdleNetwork()
+      expect(hasRscRequest).toBe(true)
     })
 
     it('should not scroll to hash when scroll={false} is set', async () => {