From 929cb696552645360a419288b4d99f67c7d414f1 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:11:09 -0700 Subject: [PATCH] fix prefetch bailout detection for nested loading segments (#67358) 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. 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. 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](https://github.com/vercel/next.js/blob/b87d8fc49983a3be568517d7ae14087749bb8ce3/packages/next/src/server/app-render/create-component-tree.tsx#L393) when determining if parallel route(s) should be rendered. --- .../walk-tree-with-flight-router-state.tsx | 8 +++++--- test/e2e/app-dir/app-prefetch/app/page.js | 3 +++ .../app/prefetch-auto/[slug]/layout.js | 12 ++++++++++++ .../app/prefetch-auto/[slug]/loading.js | 2 +- .../app/prefetch-auto/[slug]/page.js | 6 +++--- .../app-dir/app-prefetch/prefetching.test.ts | 18 ++++++++++++++++-- 6 files changed, 40 insertions(+), 9 deletions(-) diff --git a/packages/next/src/server/app-render/walk-tree-with-flight-router-state.tsx b/packages/next/src/server/app-render/walk-tree-with-flight-router-state.tsx index 3876c9356b35d..1c568b61fd6c3 100644 --- a/packages/next/src/server/app-render/walk-tree-with-flight-router-state.tsx +++ b/packages/next/src/server/app-render/walk-tree-with-flight-router-state.tsx @@ -108,14 +108,16 @@ export async function walkTreeWithFlightRouterState({ // Explicit refresh 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 = // loading.tsx has no effect on prefetching when PPR is enabled !experimental.ppr && isPrefetch && !Boolean(components.loading) && - (flightRouterState || - // If there is no flightRouterState, we need to check the entire loader tree, as otherwise we'll be only checking the root - !hasLoadingComponentInTree(loaderTree)) + !hasLoadingComponentInTree(loaderTree) if (!parentRendered && renderComponentsOnThisLevel) { const overriddenSegment = diff --git a/test/e2e/app-dir/app-prefetch/app/page.js b/test/e2e/app-dir/app-prefetch/app/page.js index 17c9aeae53c0b..ccded0f324b50 100644 --- a/test/e2e/app-dir/app-prefetch/app/page.js +++ b/test/e2e/app-dir/app-prefetch/app/page.js @@ -8,6 +8,9 @@ export default function HomePage() { To Static Page + + To Dynamic Slug Page + ) } diff --git a/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/layout.js b/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/layout.js index a9123d41fc185..f404ca3e0a91f 100644 --- a/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/layout.js +++ b/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/layout.js @@ -2,7 +2,18 @@ import Link from 'next/link' 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 }) { + const result = await getData() + return (

Layout

@@ -10,6 +21,7 @@ export default async function Layout({ children }) { Prefetch Link {children} +

{JSON.stringify(result)}

) } diff --git a/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/loading.js b/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/loading.js index 5b91c2379fa9c..9105a53b67e8f 100644 --- a/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/loading.js +++ b/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/loading.js @@ -1,3 +1,3 @@ export default function Loading() { - return

Loading Prefetch Auto

+ return

Loading Prefetch Auto

} diff --git a/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/page.js b/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/page.js index b6aa1a5033af3..23c8bdc9b5cb1 100644 --- a/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/page.js +++ b/test/e2e/app-dir/app-prefetch/app/prefetch-auto/[slug]/page.js @@ -3,7 +3,7 @@ export const dynamic = 'force-dynamic' function getData() { const res = new Promise((resolve) => { setTimeout(() => { - resolve({ message: 'Hello World!' }) + resolve({ message: 'Page Data!' }) }, 2000) }) return res @@ -13,9 +13,9 @@ export default async function Page({ params }) { const result = await getData() return ( - <> +

{JSON.stringify(params)}

{JSON.stringify(result)}

- +
) } diff --git a/test/e2e/app-dir/app-prefetch/prefetching.test.ts b/test/e2e/app-dir/app-prefetch/prefetching.test.ts index df83f886da1a1..6fabca7170f61 100644 --- a/test/e2e/app-dir/app-prefetch/prefetching.test.ts +++ b/test/e2e/app-dir/app-prefetch/prefetching.test.ts @@ -263,7 +263,8 @@ createNextDescribe( }) 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') }) @@ -297,10 +298,23 @@ createNextDescribe( }) const prefetchResponse = await response.text() - expect(prefetchResponse).not.toContain('Hello World') + expect(prefetchResponse).not.toContain('Page Data!') expect(prefetchResponse).toContain('Loading Prefetch Auto') }) + 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.each(['/force-dynamic', '/revalidate-0'])('%s', (basePath) => { it('should not re-render layout when navigating between sub-pages', async () => {