Skip to content

Commit bfa5387

Browse files
acdliteacdlite
authored
Skip request if "full" prefetch is already pending (#86405)
If a navigation happens while a prefetch for a segment is still pending, we intentionally don't wait for the prefetch to complete before sending a navigation request, because we don't know yet whether the prefetch response will be complete or partial. So, we immediately send a new navigation request for the data, even at the risk of some duplication, in order to avoid a waterfall. However, there is one case where it's safe to assume the prefetch response will be complete — when the prefetch is initiated via `<Link prefetch={true}>` (or, equivalently, the "full" option supported by router.prefetch()). In this case, the client explicitly instructs the server not to omit any dynamic or runtime data. Therefore, it's OK to skip over it during the navigation request.
1 parent f9f625b commit bfa5387

File tree

9 files changed

+177
-7
lines changed

9 files changed

+177
-7
lines changed

‎packages/next/src/client/components/segment-cache/cache.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ export type PendingSegmentCacheEntry = SegmentCacheEntryShared & {
241241
status: EntryStatus.Pending
242242
rsc: null
243243
loading: null
244-
isPartial: true
244+
isPartial: boolean
245245
promise: null | PromiseWithResolvers<FulfilledSegmentCacheEntry | null>
246246
}
247247

@@ -847,6 +847,14 @@ export function upgradeToPendingSegment(
847847
const pendingEntry: PendingSegmentCacheEntry = emptyEntry as any
848848
pendingEntry.status = EntryStatus.Pending
849849
pendingEntry.fetchStrategy = fetchStrategy
850+
851+
if (fetchStrategy === FetchStrategy.Full) {
852+
// We can assume the response will contain the full segment data. Set this
853+
// to false so we know it's OK to omit this segment from any navigation
854+
// requests that may happen while the data is still pending.
855+
pendingEntry.isPartial = false
856+
}
857+
850858
// Set the version here, since this is right before the request is initiated.
851859
// The next time the global cache version is incremented, the entry will
852860
// effectively be evicted. This happens before initiating the request, rather

‎packages/next/src/client/components/segment-cache/navigation.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -444,10 +444,17 @@ function readRenderSnapshotFromCache(
444444
loading = promiseForFulfilledEntry.then((entry) =>
445445
entry !== null ? entry.loading : null
446446
)
447-
// Since we don't know yet whether the segment is partial or fully
448-
// static, we must assume it's partial; we can't skip the
449-
// dynamic request.
450-
isPartial = true
447+
// Because the request is still pending, we typically don't know yet
448+
// whether the response will be partial. We shouldn't skip this segment
449+
// during the dynamic navigation request. Otherwise, we might need to
450+
// do yet another request to fill in the remaining data, creating
451+
// a waterfall.
452+
//
453+
// The one exception is if this segment is being fetched with via
454+
// prefetch={true} (i.e. the "force stale" or "full" strategy). If so,
455+
// we can assume the response will be full. This field is set to `false`
456+
// for such segments.
457+
isPartial = segmentEntry.isPartial
451458
break
452459
}
453460
case EntryStatus.Empty:
@@ -508,7 +515,7 @@ function readHeadSnapshotFromCache(
508515
rsc = promiseForFulfilledEntry.then((entry) =>
509516
entry !== null ? entry.rsc : null
510517
)
511-
isPartial = true
518+
isPartial = segmentEntry.isPartial
512519
break
513520
}
514521
case EntryStatus.Empty:

‎test/e2e/app-dir/segment-cache/force-stale/app/dynamic/page.tsx‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import { connection } from 'next/server'
2+
import { Suspense } from 'react'
3+
4+
async function PageContent() {
5+
await connection()
6+
return <div id="dynamic-page-content">Dynamic page content</div>
7+
}
8+
9+
export default async function Page() {
10+
return (
11+
<Suspense fallback={<div>Loading...</div>}>
12+
<PageContent />
13+
</Suspense>
14+
)
15+
}

‎test/e2e/app-dir/segment-cache/force-stale/app/layout.tsx‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
export default function RootLayout({
2+
children,
3+
}: {
4+
children: React.ReactNode
5+
}) {
6+
return (
7+
<html lang="en">
8+
<body>{children}</body>
9+
</html>
10+
)
11+
}

‎test/e2e/app-dir/segment-cache/force-stale/app/page.tsx‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import { LinkAccordion } from '../components/link-accordion'
2+
3+
export default function Page() {
4+
return (
5+
<LinkAccordion href="/dynamic" prefetch={true}>
6+
Dynamic page
7+
</LinkAccordion>
8+
)
9+
}

‎test/e2e/app-dir/segment-cache/force-stale/components/link-accordion.tsx‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
'use client'
2+
3+
import Link, { type LinkProps } from 'next/link'
4+
import { useState } from 'react'
5+
6+
export function LinkAccordion({
7+
href,
8+
children,
9+
prefetch,
10+
}: {
11+
href: string
12+
children: React.ReactNode
13+
prefetch?: LinkProps['prefetch']
14+
}) {
15+
const [isVisible, setIsVisible] = useState(false)
16+
return (
17+
<>
18+
<input
19+
type="checkbox"
20+
checked={isVisible}
21+
onChange={() => setIsVisible(!isVisible)}
22+
data-link-accordion={href}
23+
/>
24+
{isVisible ? (
25+
<Link href={href} prefetch={prefetch}>
26+
{children}
27+
</Link>
28+
) : (
29+
<>{children} (link is hidden)</>
30+
)}
31+
</>
32+
)
33+
}

‎test/e2e/app-dir/segment-cache/force-stale/force-stale.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import { nextTestSetup } from 'e2e-utils'
2+
import type * as Playwright from 'playwright'
3+
import { createRouterAct } from 'router-act'
4+
5+
describe('force stale', () => {
6+
const { next, isNextDev } = nextTestSetup({
7+
files: __dirname,
8+
})
9+
if (isNextDev) {
10+
test('prefetching is disabled in dev', () => {})
11+
return
12+
}
13+
14+
it(
15+
'during a navigation, don\'t request segments that have a pending "full" ' +
16+
'prefetch already in progress',
17+
async () => {
18+
let act: ReturnType<typeof createRouterAct>
19+
const browser = await next.browser('/', {
20+
beforePageLoad(p: Playwright.Page) {
21+
act = createRouterAct(p)
22+
},
23+
})
24+
25+
await act(
26+
async () => {
27+
// Reveal a link to a dynamic page. The Link has prefetch={true}, so the
28+
// full page data is prefetched, including dynamic content.
29+
const toggleLinkVisibility = await browser.elementByCss(
30+
'input[data-link-accordion="/dynamic"]'
31+
)
32+
await act(async () => await toggleLinkVisibility.click(), {
33+
includes: 'Dynamic page content',
34+
// Block the data from loading into the client so we can test what
35+
// happens if we request the same segment again during a navigation.
36+
block: true,
37+
})
38+
39+
// Initiate a navigation to the dynamic page. Even though the dynamic
40+
// content from the prefetch hasn't loaded yet, the router should not
41+
// request the same segment again, because it knows the data it
42+
// receives from the prefetch will be complete. This assumption is
43+
// _only_ correct for "full" prefetches, because we explicitly instruct
44+
// the server not to omit any dynamic or runtime data.
45+
const link = await browser.elementByCss('a[href="/dynamic"]')
46+
await link.click()
47+
},
48+
// There should have been no additional requests upon navigation
49+
'no-requests'
50+
)
51+
52+
// The data succesfully streams in.
53+
const content = await browser.elementById('dynamic-page-content')
54+
expect(await content.text()).toBe('Dynamic page content')
55+
}
56+
)
57+
})

‎test/e2e/app-dir/segment-cache/force-stale/next.config.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
/**
2+
* @type {import('next').NextConfig}
3+
*/
4+
const nextConfig = {
5+
cacheComponents: true,
6+
}
7+
8+
module.exports = nextConfig

‎test/lib/router-act.ts‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,29 @@ export function createRouterAct(
7777
for (let attempt = 0; attempt < maxRetries; attempt++) {
7878
try {
7979
await page.evaluate(
80-
() => new Promise<void>((res) => requestIdleCallback(() => res()))
80+
() =>
81+
new Promise<void>((res) =>
82+
requestIdleCallback(() => res(), {
83+
// Add a timeout option to prevents the callback from being
84+
// backgrounded indefinitely. Not sure why this happens but
85+
// without it, the callback will never fire.
86+
//
87+
// Note that this does not delay the callback from firing.
88+
// It should still fire pretty much "immediately". It's just a
89+
// safeguard in case the idle callback queue is not fired within
90+
// a reasonable amount of time. It really shouldn't
91+
// be necessary.
92+
//
93+
// TODO: I'm getting increasingly frustrated by how flaky
94+
// Playwright's APIs are. At this point I'm convinced we should
95+
// rewrite the whole router-act module to an equivalent
96+
// implementation that runs directly in the browser, by
97+
// injecting a script into the page. Since we only use it for
98+
// our own contrived e2e test apps, we can just import the
99+
// script into each test app that needs it.
100+
timeout: 100,
101+
})
102+
)
81103
)
82104
return
83105
} catch (err) {

0 commit comments

Comments
 (0)