fix prefetch bailout detection for nested loading segments (#67358)

### What
When PPR is off, app router prefetches will render the component tree up
until it encounters a `loading` segment, at which point it'll just
return some metadata about the segment and won't do any further
rendering. This is an optimization to ensure prefetches are lightweight
and don't potentially invoke expensive dynamic subtrees. However,
there's a bug in this logic that is causing it to bail unexpectedly if a
segment deeper in the tree contained a `loading.js` file.

This would mean the loading state wouldn't be triggered until the second
request for the full RSC data is initiated, resulting in an unintended
delta between when a link is clicked and the loading state is shown.

### Why
The `shouldSkipComponentTree` flag was incorrectly being set to `true`
even if the `loading.js` segment appeared deeper in the tree. Prefetch
requests from the client will always contain `FlightRouterState`, so the
logic to check for loading deeper in the tree will always be missed.


### How
This removes the `flightRouterState` check as it doesn't make sense:
prefetches will currently _always_ include the `flightRouterState`,
causing this to always short-circuit. I believe that this check is
vestigial from when we were generating static `prefetch.rsc` outputs
which is no longer the case.

This mirrors a [similar
check](b87d8fc499/packages/next/src/server/app-render/create-component-tree.tsx (L393))
when determining if parallel route(s) should be rendered.
This commit is contained in:
Zack Tanner 2024-07-01 16:11:09 -07:00 committed by GitHub
parent 7523f327e0
commit 2fbdac6f82
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 40 additions and 10 deletions

View file

@ -111,14 +111,15 @@ export async function walkTreeWithFlightRouterState({
// Explicit refresh // Explicit refresh
flightRouterState[3] === 'refetch' flightRouterState[3] === 'refetch'
// Pre-PPR, the `loading` component signals to the router how deep to render the component tree
// to ensure prefetches are quick and inexpensive. If there's no `loading` component anywhere in the tree being rendered,
// the prefetch will be short-circuited to avoid requesting a potentially very expensive subtree. If there's a `loading`
// somewhere in the tree, we'll recursively render the component tree up until we encounter that loading component, and then stop.
const shouldSkipComponentTree = const shouldSkipComponentTree =
// loading.tsx has no effect on prefetching when PPR is enabled
!experimental.isRoutePPREnabled && !experimental.isRoutePPREnabled &&
isPrefetch && isPrefetch &&
!Boolean(components.loading) && !Boolean(components.loading) &&
(flightRouterState || !hasLoadingComponentInTree(loaderTree)
// If there is no flightRouterState, we need to check the entire loader tree, as otherwise we'll be only checking the root
!hasLoadingComponentInTree(loaderTree))
if (!parentRendered && renderComponentsOnThisLevel) { if (!parentRendered && renderComponentsOnThisLevel) {
const overriddenSegment = const overriddenSegment =

View file

@ -8,6 +8,9 @@ export default function HomePage() {
<Link href="/static-page" id="to-static-page"> <Link href="/static-page" id="to-static-page">
To Static Page To Static Page
</Link> </Link>
<Link href="/prefetch-auto/foobar" id="to-dynamic-page">
To Dynamic Slug Page
</Link>
</> </>
) )
} }

View file

@ -2,7 +2,18 @@ import Link from 'next/link'
export const dynamic = 'force-dynamic' export const dynamic = 'force-dynamic'
function getData() {
const res = new Promise((resolve) => {
setTimeout(() => {
resolve({ message: 'Layout Data!' })
}, 2000)
})
return res
}
export default async function Layout({ children }) { export default async function Layout({ children }) {
const result = await getData()
return ( return (
<div> <div>
<h1>Layout</h1> <h1>Layout</h1>
@ -10,6 +21,7 @@ export default async function Layout({ children }) {
Prefetch Link Prefetch Link
</Link> </Link>
{children} {children}
<h3>{JSON.stringify(result)}</h3>
</div> </div>
) )
} }

View file

@ -1,3 +1,3 @@
export default function Loading() { export default function Loading() {
return <h1>Loading Prefetch Auto</h1> return <h1 id="loading-text">Loading Prefetch Auto</h1>
} }

View file

@ -3,7 +3,7 @@ export const dynamic = 'force-dynamic'
function getData() { function getData() {
const res = new Promise((resolve) => { const res = new Promise((resolve) => {
setTimeout(() => { setTimeout(() => {
resolve({ message: 'Hello World!' }) resolve({ message: 'Page Data!' })
}, 2000) }, 2000)
}) })
return res return res
@ -13,9 +13,9 @@ export default async function Page({ params }) {
const result = await getData() const result = await getData()
return ( return (
<> <div id="prefetch-auto-page-data">
<h3>{JSON.stringify(params)}</h3> <h3>{JSON.stringify(params)}</h3>
<h3>{JSON.stringify(result)}</h3> <h3>{JSON.stringify(result)}</h3>
</> </div>
) )
} }

View file

@ -220,7 +220,8 @@ describe('app dir - prefetching', () => {
}) })
const prefetchResponse = await response.text() const prefetchResponse = await response.text()
expect(prefetchResponse).not.toContain('Hello World') expect(prefetchResponse).toContain('Page Data!')
expect(prefetchResponse).not.toContain('Layout Data!')
expect(prefetchResponse).not.toContain('Loading Prefetch Auto') expect(prefetchResponse).not.toContain('Loading Prefetch Auto')
}) })
@ -254,7 +255,7 @@ describe('app dir - prefetching', () => {
}) })
const prefetchResponse = await response.text() const prefetchResponse = await response.text()
expect(prefetchResponse).not.toContain('Hello World') expect(prefetchResponse).not.toContain('Page Data!')
expect(prefetchResponse).toContain('Loading Prefetch Auto') expect(prefetchResponse).toContain('Loading Prefetch Auto')
}) })
@ -275,6 +276,19 @@ describe('app dir - prefetching', () => {
) )
}) })
it('should immediately render the loading state for a dynamic segment when fetched from higher up in the tree', async () => {
const browser = await next.browser('/')
const loadingText = await browser
.elementById('to-dynamic-page')
.click()
.waitForElementByCss('#loading-text')
.text()
expect(loadingText).toBe('Loading Prefetch Auto')
await browser.waitForElementByCss('#prefetch-auto-page-data')
})
describe('dynamic rendering', () => { describe('dynamic rendering', () => {
describe.each(['/force-dynamic', '/revalidate-0'])('%s', (basePath) => { describe.each(['/force-dynamic', '/revalidate-0'])('%s', (basePath) => {
it('should not re-render layout when navigating between sub-pages', async () => { it('should not re-render layout when navigating between sub-pages', async () => {