Fix defaultShouldRevalidate when using single fetch (Remix PR 10139) (#12170)

Author: brophdawg11

integration/single-fetch-test.ts

@@ -2108,6 +2108,98 @@ test.describe("single-fetch", () => {
       ).toBe(true);
     });
 
+    test("provides the proper defaultShouldRevalidate value", async ({
+      page,
+    }) => {
+      let fixture = await createFixture({
+        files: {
+          ...files,
+          "app/routes/_index.tsx": js`
+            import { Link } from 'react-router';
+            export default function Component() {
+              return <Link to="/parent/a">Go to /parent/a</Link>;
+            }
+          `,
+          "app/routes/parent.tsx": js`
+            import { Link, Outlet, useLoaderData } from 'react-router';
+            let count = 0;
+            export function loader({ request }) {
+              return { count: ++count };
+            }
+            export function shouldRevalidate({ defaultShouldRevalidate }) {
+              return defaultShouldRevalidate;
+            }
+            export default function Component() {
+              return (
+                <>
+                  <p id="parent">Parent Count: {useLoaderData().count}</p>
+                  <Link to="/parent/a">Go to /parent/a</Link>
+                  <Link to="/parent/b">Go to /parent/b</Link>
+                  <Outlet/>
+                </>
+              );
+            }
+          `,
+          "app/routes/parent.a.tsx": js`
+            import { useLoaderData } from 'react-router';
+            let count = 0;
+            export function loader({ request }) {
+              return { count: ++count };
+            }
+            export default function Component() {
+              return <p id="a">A Count: {useLoaderData().count}</p>;
+            }
+          `,
+          "app/routes/parent.b.tsx": js`
+            import { useLoaderData } from 'react-router';
+            let count = 0;
+            export function loader({ request }) {
+              return { count: ++count };
+            }
+            export default function Component() {
+              return <p id="b">B Count: {useLoaderData().count}</p>;
+            }
+          `,
+        },
+      });
+      let appFixture = await createAppFixture(fixture);
+      let app = new PlaywrightFixture(appFixture, page);
+
+      let urls: string[] = [];
+      page.on("request", (req) => {
+        if (req.url().includes(".data")) {
+          urls.push(req.url());
+        }
+      });
+
+      await app.goto("/");
+
+      await app.clickLink("/parent/a");
+      await page.waitForSelector("#a");
+      expect(await app.getHtml("#parent")).toContain("Parent Count: 1");
+      expect(await app.getHtml("#a")).toContain("A Count: 1");
+      expect(urls.length).toBe(1);
+      expect(urls[0].endsWith("/parent/a.data")).toBe(true);
+      urls = [];
+
+      await app.clickLink("/parent/b");
+      await page.waitForSelector("#b");
+      expect(await app.getHtml("#parent")).toContain("Parent Count: 2");
+      expect(await app.getHtml("#b")).toContain("B Count: 1");
+      expect(urls.length).toBe(1);
+      // Reload the parent route
+      expect(urls[0].endsWith("/parent/b.data")).toBe(true);
+      urls = [];
+
+      await app.clickLink("/parent/a");
+      await page.waitForSelector("#a");
+      expect(await app.getHtml("#parent")).toContain("Parent Count: 3");
+      expect(await app.getHtml("#a")).toContain("A Count: 2");
+      expect(urls.length).toBe(1);
+      // Reload the parent route
+      expect(urls[0].endsWith("/parent/a.data")).toBe(true);
+    });
+
     test("does not add a _routes param for routes without loaders", async ({
       page,
     }) => {

packages/react-router/lib/dom/ssr/links.ts

@@ -168,8 +168,6 @@ export function getNewMatchesForLinks(
   location: Location,
   mode: "data" | "assets"
 ): AgnosticDataRouteMatch[] {
-  let path = parsePathPatch(page);
-
   let isNew = (match: AgnosticDataRouteMatch, index: number) => {
     if (!currentMatches[index]) return true;
     return match.route.id !== currentMatches[index].route.id;
@@ -186,48 +184,47 @@ export function getNewMatchesForLinks(
     );
   };
 
-  // NOTE: keep this mostly up-to-date w/ the transition data diff, but this
+  if (mode === "assets") {
+    return nextMatches.filter(
+      (match, index) => isNew(match, index) || matchPathChanged(match, index)
+    );
+  }
+
+  // NOTE: keep this mostly up-to-date w/ the router data diff, but this
   // version doesn't care about submissions
-  let newMatches =
-    mode === "data" && location.search !== path.search
-      ? // this is really similar to stuff in transition.ts, maybe somebody smarter
-        // than me (or in less of a hurry) can share some of it. You're the best.
-        nextMatches.filter((match, index) => {
-          let manifestRoute = manifest.routes[match.route.id];
-          if (!manifestRoute.hasLoader) {
-            return false;
-          }
-
-          if (isNew(match, index) || matchPathChanged(match, index)) {
-            return true;
-          }
-
-          if (match.route.shouldRevalidate) {
-            let routeChoice = match.route.shouldRevalidate({
-              currentUrl: new URL(
-                location.pathname + location.search + location.hash,
-                window.origin
-              ),
-              currentParams: currentMatches[0]?.params || {},
-              nextUrl: new URL(page, window.origin),
-              nextParams: match.params,
-              defaultShouldRevalidate: true,
-            });
-            if (typeof routeChoice === "boolean") {
-              return routeChoice;
-            }
-          }
-          return true;
-        })
-      : nextMatches.filter((match, index) => {
-          let manifestRoute = manifest.routes[match.route.id];
-          return (
-            (mode === "assets" || manifestRoute.hasLoader) &&
-            (isNew(match, index) || matchPathChanged(match, index))
-          );
+  // TODO: this is really similar to stuff in router.ts, maybe somebody smarter
+  // than me (or in less of a hurry) can share some of it. You're the best.
+  if (mode === "data") {
+    return nextMatches.filter((match, index) => {
+      let manifestRoute = manifest.routes[match.route.id];
+      if (!manifestRoute.hasLoader) {
+        return false;
+      }
+
+      if (isNew(match, index) || matchPathChanged(match, index)) {
+        return true;
+      }
+
+      if (match.route.shouldRevalidate) {
+        let routeChoice = match.route.shouldRevalidate({
+          currentUrl: new URL(
+            location.pathname + location.search + location.hash,
+            window.origin
+          ),
+          currentParams: currentMatches[0]?.params || {},
+          nextUrl: new URL(page, window.origin),
+          nextParams: match.params,
+          defaultShouldRevalidate: true,
         });
+        if (typeof routeChoice === "boolean") {
+          return routeChoice;
+        }
+      }
+      return true;
+    });
+  }
 
-  return newMatches;
+  return [];
 }
 
 export function getModuleLinkHrefs(
@@ -320,13 +317,6 @@ function dedupeLinkDescriptors<Descriptor extends LinkDescriptor>(
   }, [] as KeyedLinkDescriptor<Descriptor>[]);
 }
 
-// https://github.com/remix-run/history/issues/897
-function parsePathPatch(href: string) {
-  let path = parsePath(href);
-  if (path.search === undefined) path.search = "";
-  return path;
-}
-
 // Detect if this browser supports <link rel="preload"> (or has it enabled).
 // Originally added to handle the firefox `network.preload` config:
 //   https://bugzilla.mozilla.org/show_bug.cgi?id=1847811

packages/react-router/lib/dom/ssr/routes.tsx

@@ -5,6 +5,7 @@ import type {
   ActionFunctionArgs,
   LoaderFunctionArgs,
   ShouldRevalidateFunction,
+  ShouldRevalidateFunctionArgs,
 } from "../../router/utils";
 import { ErrorResponseImpl } from "../../router/utils";
 import type { RouteModule, RouteModules } from "./routeModules";
@@ -288,13 +289,11 @@ export function createClientRoutes(
         ...dataRoute,
         ...getRouteComponents(route, routeModule, isSpaMode),
         handle: routeModule.handle,
-        shouldRevalidate: needsRevalidation
-          ? wrapShouldRevalidateForHdr(
-              route.id,
-              routeModule.shouldRevalidate,
-              needsRevalidation
-            )
-          : routeModule.shouldRevalidate,
+        shouldRevalidate: getShouldRevalidateFunction(
+          routeModule,
+          route.id,
+          needsRevalidation
+        ),
       });
 
       let hasInitialData =
@@ -456,19 +455,15 @@ export function createClientRoutes(
             });
         }
 
-        if (needsRevalidation) {
-          lazyRoute.shouldRevalidate = wrapShouldRevalidateForHdr(
-            route.id,
-            mod.shouldRevalidate,
-            needsRevalidation
-          );
-        }
-
         return {
           ...(lazyRoute.loader ? { loader: lazyRoute.loader } : {}),
           ...(lazyRoute.action ? { action: lazyRoute.action } : {}),
           hasErrorBoundary: lazyRoute.hasErrorBoundary,
-          shouldRevalidate: lazyRoute.shouldRevalidate,
+          shouldRevalidate: getShouldRevalidateFunction(
+            lazyRoute,
+            route.id,
+            needsRevalidation
+          ),
           handle: lazyRoute.handle,
           // No need to wrap these in layout since the root route is never
           // loaded via route.lazy()
@@ -492,6 +487,31 @@ export function createClientRoutes(
   });
 }
 
+function getShouldRevalidateFunction(
+  route: Partial<DataRouteObject>,
+  routeId: string,
+  needsRevalidation: Set<string> | undefined
+) {
+  // During HDR we force revalidation for updated routes
+  if (needsRevalidation) {
+    return wrapShouldRevalidateForHdr(
+      routeId,
+      route.shouldRevalidate,
+      needsRevalidation
+    );
+  }
+
+  // Single fetch revalidates by default, so override the RR default value which
+  // matches the multi-fetch behavior with `true`
+  if (route.shouldRevalidate) {
+    let fn = route.shouldRevalidate;
+    return (opts: ShouldRevalidateFunctionArgs) =>
+      fn({ ...opts, defaultShouldRevalidate: true });
+  }
+
+  return route.shouldRevalidate;
+}
+
 // When an HMR / HDR update happens we opt out of all user-defined
 // revalidation logic and force a revalidation on the first call
 function wrapShouldRevalidateForHdr(