refactor(react-router): internal data strategy (#13253)

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

.changeset/yellow-mangos-impress.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+Add support for the new `unstable_shouldCallHandler`/`unstable_shouldRevalidateArgs` APIs in `dataStrategy`

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-export/hydrated-router.tsx

@@ -209,7 +209,6 @@ function createHydratedRouter({
     },
     dataStrategy: getSingleFetchDataStrategy(
       ssrInfo.manifest,
-      ssrInfo.routeModules,
       ssrInfo.context.ssr,
       ssrInfo.context.basename,
       () => router

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

@@ -139,7 +139,6 @@ function handleMiddlewareError(error: unknown, routeId: string) {
 
 export function getSingleFetchDataStrategy(
   manifest: AssetsManifest,
-  routeModules: RouteModules,
   ssr: boolean,
   basename: string | undefined,
   getRouter: () => DataRouter
@@ -152,12 +151,11 @@ export function getSingleFetchDataStrategy(
       return runMiddlewarePipeline(
         args,
         false,
-        () => singleFetchActionStrategy(request, matches, basename),
+        () => singleFetchActionStrategy(args, basename),
         handleMiddlewareError
       ) as Promise<Record<string, DataStrategyResult>>;
     }
 
-    // TODO: Enable middleware for this flow
     if (!ssr) {
       // If this is SPA mode, there won't be any loaders below root and we'll
       // disable single fetch.  We have to keep the `dataStrategy` defined for
@@ -193,15 +191,15 @@ export function getSingleFetchDataStrategy(
       //   the other end
       let foundRevalidatingServerLoader = matches.some(
         (m) =>
-          m.shouldLoad &&
+          m.unstable_shouldCallHandler() &&
           manifest.routes[m.route.id]?.hasLoader &&
           !manifest.routes[m.route.id]?.hasClientLoader
       );
       if (!foundRevalidatingServerLoader) {
         return runMiddlewarePipeline(
           args,
           false,
-          () => nonSsrStrategy(manifest, request, matches, basename),
+          () => nonSsrStrategy(args, manifest, basename),
           handleMiddlewareError
         ) as Promise<Record<string, DataStrategyResult>>;
       }
@@ -223,12 +221,10 @@ export function getSingleFetchDataStrategy(
       false,
       () =>
         singleFetchLoaderNavigationStrategy(
+          args,
           manifest,
-          routeModules,
           ssr,
           getRouter(),
-          request,
-          matches,
           basename
         ),
       handleMiddlewareError
@@ -239,11 +235,10 @@ export function getSingleFetchDataStrategy(
 // Actions are simple since they're singular calls to the server for both
 // navigations and fetchers)
 async function singleFetchActionStrategy(
-  request: Request,
-  matches: DataStrategyFunctionArgs["matches"],
+  { request, matches }: DataStrategyFunctionArgs,
   basename: string | undefined
 ) {
-  let actionMatch = matches.find((m) => m.shouldLoad);
+  let actionMatch = matches.find((m) => m.unstable_shouldCallHandler());
   invariant(actionMatch, "No action match found");
   let actionStatus: number | undefined = undefined;
   let result = await actionMatch.resolve(async (handler) => {
@@ -276,12 +271,11 @@ async function singleFetchActionStrategy(
 
 // We want to opt-out of Single Fetch when we aren't in SSR mode
 async function nonSsrStrategy(
+  { request, matches }: DataStrategyFunctionArgs,
   manifest: AssetsManifest,
-  request: Request,
-  matches: DataStrategyFunctionArgs["matches"],
   basename: string | undefined
 ) {
-  let matchesToLoad = matches.filter((m) => m.shouldLoad);
+  let matchesToLoad = matches.filter((m) => m.unstable_shouldCallHandler());
   let url = stripIndexParam(singleFetchUrl(request.url, basename));
   let init = await createRequestInit(request);
   let results: Record<string, DataStrategyResult> = {};
@@ -308,12 +302,10 @@ async function nonSsrStrategy(
 // Loaders are trickier since we only want to hit the server once, so we
 // create a singular promise for all server-loader routes to latch onto.
 async function singleFetchLoaderNavigationStrategy(
+  { request, matches }: DataStrategyFunctionArgs,
   manifest: AssetsManifest,
-  routeModules: RouteModules,
   ssr: boolean,
   router: DataRouter,
-  request: Request,
-  matches: DataStrategyFunctionArgs["matches"],
   basename: string | undefined
 ) {
   // Track which routes need a server load - in case we need to tack on a
@@ -348,32 +340,20 @@ 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 (
-            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;
-          }
+        let defaultShouldRevalidate =
+          !m.unstable_shouldRevalidateArgs ||
+          m.unstable_shouldRevalidateArgs.actionStatus == null ||
+          m.unstable_shouldRevalidateArgs.actionStatus < 400;
+        let shouldCall = m.unstable_shouldCallHandler(defaultShouldRevalidate);
+
+        if (!shouldCall) {
+          // If this route opted out of revalidation, we don't want to include
+          // it in the single fetch .data request
+          foundOptOutRoute ||=
+            m.unstable_shouldRevalidateArgs != null && // This is a revalidation,
+            manifestRoute?.hasLoader === true && // for a route with a server loader,
+            m.route.shouldRevalidate != null; // and a shouldRevalidate function
+          return;
         }
 
         // When a route has a client loader, it opts out of the singular call and
@@ -469,7 +449,7 @@ async function singleFetchLoaderFetcherStrategy(
   matches: DataStrategyFunctionArgs["matches"],
   basename: string | undefined
 ) {
-  let fetcherMatch = matches.find((m) => m.shouldLoad);
+  let fetcherMatch = matches.find((m) => m.unstable_shouldCallHandler());
   invariant(fetcherMatch, "No fetcher match found");
   let result = await fetcherMatch.resolve(async (handler) => {
     let url = stripIndexParam(singleFetchUrl(request.url, basename));