Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix erroneous RSC calls on hash changes #66434

Merged
merged 1 commit into from
Jun 1, 2024
Merged

fix erroneous RSC calls on hash changes #66434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions packages/next/src/client/components/router-reducer/handle-mutable.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these values updated because I added a new link to the test page, so the scroll is offset by 14px

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
Loading