From 7260c65e51233023b18c16d535b96a75648d7325 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Fri, 31 May 2024 16:19:05 -0700 Subject: [PATCH] fix erroneous RSC calls on hash changes --- .../router-reducer/handle-mutable.ts | 5 +- .../reducers/navigate-reducer.ts | 34 +++++++++--- .../router-reducer/router-reducer-types.ts | 1 + test/e2e/app-dir/navigation/app/hash/page.js | 5 ++ .../e2e/app-dir/navigation/navigation.test.ts | 52 ++++++++++++++----- 5 files changed, 75 insertions(+), 22 deletions(-) diff --git a/packages/next/src/client/components/router-reducer/handle-mutable.ts b/packages/next/src/client/components/router-reducer/handle-mutable.ts index 151931762439f..34dceef937a46 100644 --- a/packages/next/src/client/components/router-reducer/handle-mutable.ts +++ b/packages/next/src/client/components/router-reducer/handle-mutable.ts @@ -60,10 +60,7 @@ export function handleMutable( : state.focusAndScrollRef.apply : // If shouldScroll is false then we should not apply scroll and focus management. false, - onlyHashChange: - !!mutable.hashFragment && - state.canonicalUrl.split('#', 1)[0] === - mutable.canonicalUrl?.split('#', 1)[0], + onlyHashChange: mutable.onlyHashChange || false, hashFragment: shouldScroll ? // Empty hash should trigger default behavior of scrolling layout into view. // #top is handled in layout-router. 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 index 3d0cf8b7ee230..7a2876eeeaef6 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -156,6 +156,16 @@ function navigateReducer_noPPR( return handleExternalUrl(state, mutable, href, pendingPush) } + const updatedCanonicalUrl = canonicalUrlOverride + ? createHrefFromUrl(canonicalUrlOverride) + : href + + // Track if the navigation was only an update to the hash fragment + mutable.onlyHashChange = + !!hash && + state.canonicalUrl.split('#', 1)[0] === + updatedCanonicalUrl.split('#', 1)[0] + let currentTree = state.tree const currentCache = state.cache let scrollableSegments: FlightSegmentPath[] = [] @@ -201,12 +211,15 @@ function navigateReducer_noPPR( if ( prefetchValues.status === PrefetchCacheEntryStatus.stale && + !mutable.onlyHashChange && !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, @@ -263,9 +276,7 @@ function navigateReducer_noPPR( } mutable.patchedTree = currentTree - mutable.canonicalUrl = canonicalUrlOverride - ? createHrefFromUrl(canonicalUrlOverride) - : href + mutable.canonicalUrl = updatedCanonicalUrl mutable.pendingPush = pendingPush mutable.scrollableSegments = scrollableSegments mutable.hashFragment = hash @@ -330,6 +341,16 @@ function navigateReducer_PPR( return handleExternalUrl(state, mutable, href, pendingPush) } + const updatedCanonicalUrl = canonicalUrlOverride + ? createHrefFromUrl(canonicalUrlOverride) + : href + + // Track if the navigation was only an update to the hash fragment + mutable.onlyHashChange = + !!hash && + state.canonicalUrl.split('#', 1)[0] === + updatedCanonicalUrl.split('#', 1)[0] + let currentTree = state.tree const currentCache = state.cache let scrollableSegments: FlightSegmentPath[] = [] @@ -452,12 +473,15 @@ function navigateReducer_PPR( if ( prefetchValues.status === PrefetchCacheEntryStatus.stale && + !mutable.onlyHashChange && !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, @@ -515,9 +539,7 @@ function navigateReducer_PPR( } mutable.patchedTree = currentTree - mutable.canonicalUrl = canonicalUrlOverride - ? createHrefFromUrl(canonicalUrlOverride) - : href + mutable.canonicalUrl = updatedCanonicalUrl mutable.pendingPush = pendingPush mutable.scrollableSegments = scrollableSegments mutable.hashFragment = hash diff --git a/packages/next/src/client/components/router-reducer/router-reducer-types.ts b/packages/next/src/client/components/router-reducer/router-reducer-types.ts index 3576281b3d32e..c2f5358211b19 100644 --- a/packages/next/src/client/components/router-reducer/router-reducer-types.ts +++ b/packages/next/src/client/components/router-reducer/router-reducer-types.ts @@ -38,6 +38,7 @@ export interface Mutable { hashFragment?: string shouldScroll?: boolean preserveCustomHistoryState?: boolean + onlyHashChange?: boolean } export interface ServerActionMutable extends Mutable { diff --git a/test/e2e/app-dir/navigation/app/hash/page.js b/test/e2e/app-dir/navigation/app/hash/page.js index a1c0ab4a13f31..d88a34f3da397 100644 --- 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() { To non-existent +
+ + To 160 (with query param) + +
{items.map((item) => (
diff --git a/test/e2e/app-dir/navigation/navigation.test.ts b/test/e2e/app-dir/navigation/navigation.test.ts index 91002c89382ce..73743aefd24c5 100644 --- 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 () => { @@ -201,11 +229,11 @@ describe('app dir - navigation', () => { ) } - await checkLink(6, 94) - await checkLink(50, 710) - await checkLink(160, 2250) - await checkLink(300, 4210) - await checkLink(500, 7010) // this one is hash only (`href="#hash-500"`) + await checkLink(6, 108) + await checkLink(50, 724) + await checkLink(160, 2264) + await checkLink(300, 4224) + await checkLink(500, 7024) // this one is hash only (`href="#hash-500"`) await checkLink('top', 0) await checkLink('non-existent', 0) })