Adopt new unstable_shouldCallHandler API to fix revalidation bug on reused routes

Author: brophdawg11

.changeset/fair-weeks-beam.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+Fix single fetch bug where no revalidation request would be made when navigating upwards to a reused parent route

integration/single-fetch-test.ts

@@ -408,6 +408,80 @@ test.describe("single-fetch", () => {
     ]);
   });
 
+  test("revalidates on reused routes by default", async ({ page }) => {
+    let fixture = await createFixture({
+      files: {
+        ...files,
+        "app/routes/_index.tsx": js`
+          import { Link } from "react-router";
+          export default function Index() {
+            return <Link to="/parent">Go to Parent</Link>
+          }
+        `,
+        "app/routes/parent.tsx": js`
+          import { Link, Outlet } from "react-router";
+          import type { Route } from "./+types/parent";
+
+          let count = 0;
+          export function loader() {
+            return ++count;
+          }
+
+          export default function Parent({ loaderData }: Route.ComponentProps) {
+            return (
+              <>
+                <h1 data-parent={loaderData}>PARENT:{loaderData}</h1>
+                <Link to="/parent">Go to Parent</Link><br/>
+                <Link to="/parent/child">Go to Child</Link>
+                <Outlet />
+              </>
+            );
+          }
+        `,
+        "app/routes/parent.child.tsx": js`
+          import { Outlet } from "react-router";
+          import type { Route } from "./+types/parent";
+
+          export function loader() {
+            return "CHILD"
+          }
+
+          export default function Parent({ loaderData }: Route.ComponentProps) {
+            return <h2 data-child>{loaderData}</h2>
+          }
+        `,
+      },
+    });
+
+    let urls: string[] = [];
+    page.on("request", (req) => {
+      let url = new URL(req.url());
+      if (req.method() === "GET" && url.pathname.endsWith(".data")) {
+        urls.push(url.pathname + url.search);
+      }
+    });
+
+    let appFixture = await createAppFixture(fixture);
+    let app = new PlaywrightFixture(appFixture, page);
+    await app.goto("/", true);
+
+    await app.clickLink("/parent");
+    await page.waitForSelector('[data-parent="1"]');
+    expect(urls).toEqual(["/parent.data"]);
+    urls.length = 0;
+
+    await app.clickLink("/parent/child");
+    await page.waitForSelector("[data-child]");
+    await expect(page.locator('[data-parent="2"]')).toBeDefined();
+    expect(urls).toEqual(["/parent/child.data"]);
+    urls.length = 0;
+
+    await app.clickLink("/parent");
+    await page.waitForSelector('[data-parent="3"]');
+    expect(urls).toEqual(["/parent.data"]);
+    urls.length = 0;
+  });
+
   test("does not revalidate on 4xx/5xx action responses", async ({ page }) => {
     let fixture = await createFixture({
       files: {

integration/vite-prerender-test.ts

@@ -2341,7 +2341,6 @@ test.describe("Prerendering", () => {
       await app.clickLink("/param/1");
       await page.waitForSelector('[data-param="1"]');
       expect(await (await page.$("[data-param]"))?.innerText()).toBe("Param 1");
-      console.log("asserting", requests);
       expect(requests).toEqual(["/param/1.data"]);
       clearRequests(requests);
 
@@ -2426,7 +2425,6 @@ test.describe("Prerendering", () => {
       await app.clickLink("/param/1");
       await page.waitForSelector('[data-param="1"]');
       expect(await (await page.$("[data-param]"))?.innerText()).toBe("Param 1");
-      console.log("asserting", requests);
       expect(requests).toEqual(["/param/1.data"]);
       clearRequests(requests);
 

packages/react-router/__tests__/router/data-strategy-test.ts

@@ -577,6 +577,92 @@ describe("router dataStrategy", () => {
         child: "CHILD",
       });
     });
+
+    it("does not short circuit when there are no matchesToLoad", async () => {
+      let dataStrategy = mockDataStrategy(async ({ matches }) => {
+        let results = await Promise.all(
+          matches.map((m) => m.resolve((handler) => handler()))
+        );
+        // Don't use keyedResults since it checks for shouldLoad and this test
+        // is always loading
+        return results.reduce(
+          (acc, r, i) => Object.assign(acc, { [matches[i].route.id]: r }),
+          {}
+        );
+      });
+      let t = setup({
+        routes: [
+          {
+            path: "/",
+          },
+          {
+            id: "parent",
+            path: "/parent",
+            loader: true,
+            children: [
+              {
+                id: "child",
+                path: "child",
+                loader: true,
+              },
+            ],
+          },
+        ],
+        dataStrategy,
+      });
+
+      let A = await t.navigate("/parent");
+      await A.loaders.parent.resolve("PARENT1");
+      expect(A.loaders.parent.stub).toHaveBeenCalled();
+      expect(t.router.state.loaderData).toEqual({
+        parent: "PARENT1",
+      });
+      expect(dataStrategy.mock.calls[0][0].matches).toEqual([
+        expect.objectContaining({
+          route: expect.objectContaining({
+            id: "parent",
+          }),
+        }),
+      ]);
+
+      let B = await t.navigate("/parent/child");
+      await B.loaders.parent.resolve("PARENT2");
+      await B.loaders.child.resolve("CHILD");
+      expect(B.loaders.parent.stub).toHaveBeenCalled();
+      expect(B.loaders.child.stub).toHaveBeenCalled();
+      expect(t.router.state.loaderData).toEqual({
+        parent: "PARENT2",
+        child: "CHILD",
+      });
+      expect(dataStrategy.mock.calls[1][0].matches).toEqual([
+        expect.objectContaining({
+          route: expect.objectContaining({
+            id: "parent",
+          }),
+        }),
+        expect.objectContaining({
+          route: expect.objectContaining({
+            id: "child",
+          }),
+        }),
+      ]);
+
+      let C = await t.navigate("/parent");
+      await C.loaders.parent.resolve("PARENT3");
+      expect(C.loaders.parent.stub).toHaveBeenCalled();
+      expect(t.router.state.loaderData).toEqual({
+        parent: "PARENT3",
+      });
+      expect(dataStrategy.mock.calls[2][0].matches).toEqual([
+        expect.objectContaining({
+          route: expect.objectContaining({
+            id: "parent",
+          }),
+        }),
+      ]);
+
+      expect(dataStrategy).toHaveBeenCalledTimes(3);
+    });
   });
 
   describe("actions", () => {

packages/react-router/lib/dom/ssr/single-fetch.tsx

@@ -224,11 +224,11 @@ export function getSingleFetchDataStrategy(
       () =>
         singleFetchLoaderNavigationStrategy(
           manifest,
-          routeModules,
           ssr,
           getRouter(),
           request,
           matches,
+          args.unstable_shouldRevalidateArgs,
           basename
         ),
       handleMiddlewareError
@@ -309,11 +309,11 @@ async function nonSsrStrategy(
 // create a singular promise for all server-loader routes to latch onto.
 async function singleFetchLoaderNavigationStrategy(
   manifest: AssetsManifest,
-  routeModules: RouteModules,
   ssr: boolean,
   router: DataRouter,
   request: Request,
   matches: DataStrategyFunctionArgs["matches"],
+  shouldRevalidateArgs: DataStrategyFunctionArgs["unstable_shouldRevalidateArgs"],
   basename: string | undefined
 ) {
   // Track which routes need a server load - in case we need to tack on a
@@ -348,32 +348,21 @@ async function singleFetchLoaderNavigationStrategy(
 
         let manifestRoute = manifest.routes[m.route.id];
 
-        // Note: If this logic changes for routes that should not participate
-        // in Single Fetch, make sure you update getLowestLoadingIndex above
-        // as well
-        if (!m.shouldLoad) {
-          // If we're not yet initialized and this is the initial load, respect
-          // `shouldLoad` because we're only dealing with `clientLoader.hydrate`
-          // routes which will fall into the `clientLoader` section below.
-          if (!router.state.initialized) {
-            return;
-          }
-
-          // Otherwise, we opt out if we currently have data and a
-          // `shouldRevalidate` function.  This implies that the user opted out
-          // via `shouldRevalidate`
-          if (
+        let defaultShouldRevalidate =
+          !shouldRevalidateArgs ||
+          shouldRevalidateArgs.actionStatus == null ||
+          shouldRevalidateArgs.actionStatus < 400;
+        let shouldCall = m.unstable_shouldCallHandler(defaultShouldRevalidate);
+
+        if (!shouldCall) {
+          // Mark an opt out route if we currently have data and a `shouldRevalidate`
+          // function.   This implies that the user opted out via `shouldRevalidate`
+          // so we don't want to include it in the single fetch .data request
+          foundOptOutRoute ||=
+            manifestRoute?.hasLoader === true &&
             m.route.id in router.state.loaderData &&
-            manifestRoute &&
-            m.route.shouldRevalidate
-          ) {
-            if (manifestRoute.hasLoader) {
-              // If we have a server loader, make sure we don't include it in the
-              // single fetch .data request
-              foundOptOutRoute = true;
-            }
-            return;
-          }
+            m.route.shouldRevalidate != null;
+          return;
         }
 
         // When a route has a client loader, it opts out of the singular call and

packages/react-router/lib/router/router.ts

@@ -1987,8 +1987,10 @@ export function createRouter(init: RouterInit): Router {
 
     pendingNavigationLoadId = ++incrementingLoadId;
 
-    // Short circuit if we have no loaders to run
+    // Short circuit if we have no loaders to run, unless there's a custom dataStrategy
+    // since they may have different revalidation rules (i.e., single fetch)
     if (
+      !init.dataStrategy &&
       !dsMatches.some((m) => m.shouldLoad) &&
       revalidatingFetchers.length === 0
     ) {
@@ -4176,8 +4178,9 @@ export function createStaticHandler(
       });
     }
 
-    // Short circuit if we have no loaders to run (query())
-    if (!dsMatches.some((m) => m.shouldLoad)) {
+    // Short circuit if we have no loaders to run, unless there's a custom dataStrategy
+    // since they may have different revalidation rules (i.e., single fetch)
+    if (!dataStrategy && !dsMatches.some((m) => m.shouldLoad)) {
       return {
         matches,
         // Add a null for all matched routes for proper revalidation on the client