Skip to content

Commit

Permalink
fix erroneous RSC calls on hash changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@ztanner
ztanner committed Jun 1, 2024
1 parent b39ae62 commit 7260c65
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 22 deletions.
5 changes: 1 addition & 4 deletions packages/next/src/client/components/router-reducer/handle-mutable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
34 changes: 28 additions & 6 deletions packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = []
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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[] = []
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/client/components/router-reducer/router-reducer-types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export interface Mutable {
hashFragment?: string
shouldScroll?: boolean
preserveCustomHistoryState?: boolean
onlyHashChange?: boolean
}

export interface ServerActionMutable extends Mutable {
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/app-dir/navigation/app/hash/page.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}>
Expand Down
52 changes: 40 additions & 12 deletions test/e2e/app-dir/navigation/navigation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})

Expand Down Expand Up @@ -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,
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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)
})
Expand Down

0 comments on commit 7260c65

Please sign in to comment.