fix PPR navigations when visiting route with seeded cache (vercel#67439)

ztanner · web-flow · commit 1c1dc4763114 · 2024-07-09T12:54:37.000-07:00
During SSR, we walk the component tree and pass and immediately seed the
router cache with the tree.

We also seed the prefetch cache for the initial route with the same tree
data because navigations events will apply the data from the prefetch
cache. The rationale for this is that since we already have the data, we
might as well seed it in the prefetch cache, to save a server round trip
later.

The way we're seeding the cache introduced a bug when PPR is turned on:
PPR expects that each element in `FlightDataPath` is of length 3,
corresponding with: `[PrefetchedTree, SeedData, Head]`. However, we were
seeding it with 4 items: `[Segment, PrefetchedTree, SeedData, Head]`.
When the forked router implementation sees anything other than 3 items,
it reverts back to the pre-PPR behavior, which means the data will be
lazy-fetched during render rather than eagerly in the reducer.

As a result, when navigating back to the page that was seeded during
SSR, it wouldn't immediately apply the static parts to the UI: it'd
block until the dynamic render finished.

This PR also includes a small change to ensure reused tasks don’t
trigger a dynamic request. A navigation test was failing because this PR
fixed a bug that was causing it to hit the non-PPR behavior, which
already handles this correctly.

I've added an explicit test for this in PPR. I've also excluded all of
the `client-cache` tests from PPR, as many of them were already failing
and aren't relevant to PPR, since PPR doesn't currently make use of the
`staleTime`/ cache heuristics that the regular router does.
diff --git a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts
@@ -105,7 +105,9 @@ export function createInitialRouterState({
       location.origin
     )
 
-    const initialFlightData: FlightData = [['', initialTree, null, null]]
+    const initialFlightData: FlightData = [
+      [initialTree, initialSeedData, initialHead],
+    ]
     createPrefetchCacheEntryForInitialLoad({
       url,
       kind: PrefetchKind.AUTO,
diff --git a/packages/next/src/client/components/router-reducer/ppr-navigations.ts b/packages/next/src/client/components/router-reducer/ppr-navigations.ts
@@ -67,7 +67,8 @@ export function updateCacheNodeOnNavigation(
   oldRouterState: FlightRouterState,
   newRouterState: FlightRouterState,
   prefetchData: CacheNodeSeedData,
-  prefetchHead: React.ReactNode
+  prefetchHead: React.ReactNode,
+  onlyHashChange: boolean
 ): Task | null {
   // Diff the old and new trees to reuse the shared layouts.
   const oldRouterStateChildren = oldRouterState[1]
@@ -98,6 +99,17 @@ export function updateCacheNodeOnNavigation(
     [parallelRouteKey: string]: FlightRouterState
   } = {}
   let taskChildren = null
+
+  // For most navigations, we need to issue a "dynamic" request to fetch the
+  // full RSC data from the server since during rendering, we'll only serve
+  // the prefetch shell. For some navigations, we re-use the existing cache node
+  // (via `spawnReusedTask`), and don't actually need fresh data from the server.
+  // In those cases, we use this `needsDynamicRequest` flag to return a `null`
+  // cache node, which signals to the caller that we don't need to issue a
+  // dynamic request. We start off with a `false` value, and then for each parallel
+  // route, we set it to `true` if we encounter a segment that needs a dynamic request.
+  let needsDynamicRequest = false
+
   for (let parallelRouteKey in newRouterStateChildren) {
     const newRouterStateChild: FlightRouterState =
       newRouterStateChildren[parallelRouteKey]
@@ -119,7 +131,12 @@ export function updateCacheNodeOnNavigation(
         : undefined
 
     let taskChild: Task | null
-    if (newSegmentChild === PAGE_SEGMENT_KEY) {
+    if (onlyHashChange && oldRouterStateChild !== undefined) {
+      // If only the hash fragment changed, we can re-use the existing cache.
+      // We spawn a "task" just to keep track of the updated router state; unlike most, it's
+      // already fulfilled and won't be affected by the dynamic response.
+      taskChild = spawnReusedTask(oldRouterStateChild)
+    } else if (newSegmentChild === PAGE_SEGMENT_KEY) {
       // This is a leaf segment — a page, not a shared layout. We always apply
       // its data.
       taskChild = spawnPendingTask(
@@ -164,7 +181,8 @@ export function updateCacheNodeOnNavigation(
             oldRouterStateChild,
             newRouterStateChild,
             prefetchDataChild,
-            prefetchHead
+            prefetchHead,
+            onlyHashChange
           )
         } else {
           // The server didn't send any prefetch data for this segment. This
@@ -204,6 +222,9 @@ export function updateCacheNodeOnNavigation(
         const newSegmentMapChild: ChildSegmentMap = new Map(oldSegmentMapChild)
         newSegmentMapChild.set(newSegmentKeyChild, newCacheNodeChild)
         prefetchParallelRoutes.set(parallelRouteKey, newSegmentMapChild)
+        // a non-null taskChild.node means we're waiting for a dynamic request to
+        // fill in the missing data
+        needsDynamicRequest = true
       }
 
       // The child tree's route state may be different from the prefetched
@@ -245,7 +266,9 @@ export function updateCacheNodeOnNavigation(
       newRouterState,
       patchedRouterStateChildren
     ),
-    node: newCacheNode,
+    // Only return the new cache node if there are pending tasks that need to be resolved
+    // by the dynamic data from the server. If they don't, we don't need to trigger a dynamic request.
+    node: needsDynamicRequest ? newCacheNode : null,
     children: taskChildren,
   }
 }
diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts
@@ -417,9 +417,10 @@ function navigateReducer_PPR(
               currentTree,
               prefetchedTree,
               seedData,
-              head
+              head,
+              mutable.onlyHashChange
             )
-            if (task !== null && task.node !== null) {
+            if (task !== null) {
               // We've created a new Cache Node tree that contains a prefetched
               // version of the next page. This can be rendered instantly.
 
@@ -430,32 +431,37 @@ function navigateReducer_PPR(
               const patchedRouterState: FlightRouterState = task.route
               newTree = patchedRouterState
 
-              const newCache = task.node
-
-              // The prefetched tree has dynamic holes in it. We initiate a
-              // dynamic request to fill them in.
-              //
-              // Do not block on the result. We'll immediately render the Cache
-              // Node tree and suspend on the dynamic parts. When the request
-              // comes in, we'll fill in missing data and ping React to
-              // re-render. Unlike the lazy fetching model in the non-PPR
-              // implementation, this is modeled as a single React update +
-              // streaming, rather than multiple top-level updates. (However,
-              // even in the new model, we'll still need to sometimes update the
-              // root multiple times per navigation, like if the server sends us
-              // a different response than we expected. For now, we revert back
-              // to the lazy fetching mechanism in that case.)
-              listenForDynamicRequest(
-                task,
-                fetchServerResponse(
-                  url,
-                  currentTree,
-                  state.nextUrl,
-                  state.buildId
+              // It's possible that `updateCacheNodeOnNavigation` only spawned tasks to reuse the existing cache,
+              // in which case `task.node` will be null, signaling we don't need to wait for a dynamic request
+              // and can simply apply the patched `FlightRouterState`.
+              if (task.node !== null) {
+                const newCache = task.node
+
+                // The prefetched tree has dynamic holes in it. We initiate a
+                // dynamic request to fill them in.
+                //
+                // Do not block on the result. We'll immediately render the Cache
+                // Node tree and suspend on the dynamic parts. When the request
+                // comes in, we'll fill in missing data and ping React to
+                // re-render. Unlike the lazy fetching model in the non-PPR
+                // implementation, this is modeled as a single React update +
+                // streaming, rather than multiple top-level updates. (However,
+                // even in the new model, we'll still need to sometimes update the
+                // root multiple times per navigation, like if the server sends us
+                // a different response than we expected. For now, we revert back
+                // to the lazy fetching mechanism in that case.)
+                listenForDynamicRequest(
+                  task,
+                  fetchServerResponse(
+                    url,
+                    currentTree,
+                    state.nextUrl,
+                    state.buildId
+                  )
                 )
-              )
 
-              mutable.cache = newCache
+                mutable.cache = newCache
+              }
             } else {
               // Nothing changed, so reuse the old cache.
               // TODO: What if the head changed but not any of the segment data?
diff --git a/test/e2e/app-dir/app/index.test.ts b/test/e2e/app-dir/app/index.test.ts
@@ -717,9 +717,11 @@ describe('app dir - basic', () => {
         await browser.waitForElementByCss('#render-id')
         expect(await browser.eval('window.history.length')).toBe(2)
 
-        // Get the ID again, and compare, they should be the same.
-        const thirdID = await browser.elementById('render-id').text()
-        expect(thirdID).not.toBe(firstID)
+        await retry(async () => {
+          // Get the ID again, and compare, they should be the same.
+          const thirdID = await browser.elementById('render-id').text()
+          expect(thirdID).not.toBe(firstID)
+        })
 
         // verify that the flag is still set
         expect(await browser.eval('window.__nextSoftPushTest')).toBe(1)
diff --git a/test/e2e/app-dir/ppr-navigations/prefetch-navigation/prefetch-navigation.test.ts b/test/e2e/app-dir/ppr-navigations/prefetch-navigation/prefetch-navigation.test.ts
@@ -62,5 +62,14 @@ describe('prefetch-navigation', () => {
 
     // Resolve the request to allow the page to finish loading
     await rscRequestPromise.get('/catch-all/2').resolve()
+
+    // Now go back to the first page to make sure that the server seeded
+    // prefetch cache entry behaves the same as one that is retrieved
+    // from the client prefetch.
+    await browser.elementByCss('a[href="/catch-all/1"]').click()
+    await browser.waitForElementByCss('#dynamic-page-1')
+
+    targetPageParams = await browser.elementById('params').text()
+    expect(targetPageParams).toBe('Params: {"slug":["1"]}')
   })
 })
diff --git a/test/ppr-tests-manifest.json b/test/ppr-tests-manifest.json
@@ -20,18 +20,6 @@
         "app-dir static/dynamic handling should output debug info for static bailouts"
       ]
     },
-    "test/e2e/app-dir/app-client-cache/client-cache.original.test.ts": {
-      "failed": [
-        "app dir client cache semantics (30s/5min) prefetch={undefined} - default should re-use the full cache for only 30 seconds",
-        "app dir client cache semantics (30s/5min) prefetch={undefined} - default should renew the 30s cache once the data is revalidated",
-        "app dir client cache semantics (30s/5min) prefetch={undefined} - default should refetch the full page after 5 mins"
-      ]
-    },
-    "test/e2e/app-dir/app-client-cache/client-cache.defaults.test.ts": {
-      "failed": [
-        "app dir client cache semantics (default semantics) prefetch={undefined} - default should refetch the full page after 5 mins"
-      ]
-    },
     "test/e2e/app-dir/headers-static-bailout/headers-static-bailout.test.ts": {
       "failed": [
         "headers-static-bailout it provides a helpful link in case static generation bailout is uncaught"
@@ -92,7 +80,9 @@
       "test/e2e/app-dir/ppr-*/**/*",
       "test/e2e/app-dir/app-prefetch*/**/*",
       "test/e2e/app-dir/searchparams-static-bailout/searchparams-static-bailout.test.ts",
-      "test/e2e/app-dir/app-client-cache/client-cache.experimental.test.ts"
+      "test/e2e/app-dir/app-client-cache/client-cache.experimental.test.ts",
+      "test/e2e/app-dir/app-client-cache/client-cache.original.test.ts",
+      "test/e2e/app-dir/app-client-cache/client-cache.defaults.test.ts"
     ]
   }
 }
