feat: revalidate loaders only when a change to one is detected (#6135)

.changeset/hdr-revalidate-only-when-needed.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/react": patch
+---
+
+Revalidate loaders only when a change to one is detected.

integration/hmr-test.ts

@@ -155,6 +155,9 @@ let fixture = (options: {
 
     "app/routes/_index.tsx": js`
       import { useLoaderData } from "@remix-run/react";
+      export function shouldRevalidate(args) {
+        return args.defaultShouldRevalidate;
+      }
       export default function Index() {
         const t = useLoaderData();
         return (
@@ -219,6 +222,13 @@ test("HMR", async ({ page }) => {
   // uncomment for debugging
   // page.on("console", (msg) => console.log(msg.text()));
   page.on("pageerror", (err) => console.log(err.message));
+  let dataRequests = 0;
+  page.on("request", (request) => {
+    let url = new URL(request.url());
+    if (url.searchParams.has("_data")) {
+      dataRequests++;
+    }
+  });
 
   let portRange = makeRange(3080, 3099);
   let appServerPort = await getPort({ port: portRange });
@@ -276,6 +286,9 @@ test("HMR", async ({ page }) => {
     let newIndex = `
       import { useLoaderData } from "@remix-run/react";
       import styles from "~/styles.module.css";
+      export function shouldRevalidate(args) {
+        return args.defaultShouldRevalidate;
+      }
       export default function Index() {
         const t = useLoaderData();
         return (
@@ -289,6 +302,7 @@ test("HMR", async ({ page }) => {
 
     // detect HMR'd content and style changes
     await page.waitForLoadState("networkidle");
+
     let h1 = page.getByText("Changed");
     await h1.waitFor({ timeout: HMR_TIMEOUT_MS });
     expect(h1).toHaveCSS("color", "rgb(255, 255, 255)");
@@ -305,13 +319,19 @@ test("HMR", async ({ page }) => {
     expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf");
     await page.waitForSelector(`#root-counter:has-text("inc 1")`);
 
+    // We should not have done any revalidation yet as only UI has changed
+    expect(dataRequests).toBe(0);
+
     // add loader
     let withLoader1 = `
       import { json } from "@remix-run/node";
       import { useLoaderData } from "@remix-run/react";
 
-      export let loader = () => json({ hello: "world" })
+      export let loader = () => json({ hello: "world" });
 
+      export function shouldRevalidate(args) {
+        return args.defaultShouldRevalidate;
+      }
       export default function Index() {
         let { hello } = useLoaderData<typeof loader>();
         return (
@@ -322,10 +342,14 @@ test("HMR", async ({ page }) => {
       }
     `;
     fs.writeFileSync(indexPath, withLoader1);
+    await page.waitForLoadState("networkidle");
+
     await page.getByText("Hello, world").waitFor({ timeout: HMR_TIMEOUT_MS });
     expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf");
     await page.waitForSelector(`#root-counter:has-text("inc 1")`);
 
+    expect(dataRequests).toBe(1);
+
     let withLoader2 = `
       import { json } from "@remix-run/node";
       import { useLoaderData } from "@remix-run/react";
@@ -334,6 +358,9 @@ test("HMR", async ({ page }) => {
         return json({ hello: "planet" })
       }
 
+      export function shouldRevalidate(args) {
+        return args.defaultShouldRevalidate;
+      }
       export default function Index() {
         let { hello } = useLoaderData<typeof loader>();
         return (
@@ -344,10 +371,14 @@ test("HMR", async ({ page }) => {
       }
     `;
     fs.writeFileSync(indexPath, withLoader2);
+    await page.waitForLoadState("networkidle");
+
     await page.getByText("Hello, planet").waitFor({ timeout: HMR_TIMEOUT_MS });
     expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf");
     await page.waitForSelector(`#root-counter:has-text("inc 1")`);
 
+    expect(dataRequests).toBe(2);
+
     // change shared component
     let updatedCounter = `
       import * as React from "react";
@@ -388,6 +419,10 @@ test("HMR", async ({ page }) => {
     aboutCounter = await page.waitForSelector(
       `#about-counter:has-text("inc 0")`
     );
+
+    // This should not have triggered any revalidation but our detection is
+    // failing for x-module changes for route module imports
+    // expect(dataRequests).toBe(2);
   } catch (e) {
     console.log("stdout begin -----------------------");
     console.log(devStdout());

packages/remix-react/browser.tsx

@@ -13,7 +13,7 @@ import {
 } from "./errorBoundaries";
 import { deserializeErrors } from "./errors";
 import type { RouteModules } from "./routeModules";
-import { createClientRoutes } from "./routes";
+import { createClientRoutes, createClientRoutesWithHMRRevalidationOptOut } from "./routes";
 
 /* eslint-disable prefer-let/prefer-let */
 declare global {
@@ -44,12 +44,18 @@ declare global {
 }
 
 let router: Router;
-let hmrAbortController: AbortController;
+let hmrAbortController: AbortController | undefined;
 
 if (import.meta && import.meta.hot) {
   import.meta.hot.accept(
     "remix:manifest",
-    async (newManifest: EntryContext["manifest"]) => {
+    async ({
+      assetsManifest,
+      needsRevalidation,
+    }: {
+      assetsManifest: EntryContext["manifest"];
+      needsRevalidation: boolean;
+    }) => {
       let routeIds = [
         ...new Set(
           router.state.matches
@@ -58,6 +64,12 @@ if (import.meta && import.meta.hot) {
         ),
       ];
 
+      if (hmrAbortController) {
+        hmrAbortController.abort();
+      }
+      hmrAbortController = new AbortController();
+      let signal = hmrAbortController.signal;
+
       // Load new route modules that we've seen.
       let newRouteModules = Object.assign(
         {},
@@ -66,12 +78,12 @@ if (import.meta && import.meta.hot) {
           (
             await Promise.all(
               routeIds.map(async (id) => {
-                if (!newManifest.routes[id]) {
+                if (!assetsManifest.routes[id]) {
                   return null;
                 }
                 let imported = await import(
-                  newManifest.routes[id].module +
-                    `?t=${newManifest.hmr?.timestamp}`
+                  assetsManifest.routes[id].module +
+                    `?t=${assetsManifest.hmr?.timestamp}`
                 );
                 return [
                   id,
@@ -101,29 +113,28 @@ if (import.meta && import.meta.hot) {
 
       Object.assign(window.__remixRouteModules, newRouteModules);
       // Create new routes
-      let routes = createClientRoutes(
-        newManifest.routes,
+      let routes = createClientRoutesWithHMRRevalidationOptOut(
+        needsRevalidation,
+        assetsManifest.routes,
         window.__remixRouteModules,
-        window.__remixContext.future
+        window.__remixContext.future,
       );
 
       // This is temporary API and will be more granular before release
       router._internalSetRoutes(routes);
 
-      if (hmrAbortController) {
-        hmrAbortController.abort();
-      }
-      hmrAbortController = new AbortController();
-      let signal = hmrAbortController.signal;
       // Wait for router to be idle before updating the manifest and route modules
       // and triggering a react-refresh
       let unsub = router.subscribe((state) => {
-        if (state.revalidation === "idle" && !signal.aborted) {
+        if (state.revalidation === "idle") {
           unsub();
-          // TODO: Handle race conditions here. Should abort if a new update
-          // comes in while we're waiting for the router to be idle.
-          Object.assign(window.__remixManifest, newManifest);
-          window.$RefreshRuntime$.performReactRefresh();
+          // Abort if a new update comes in while we're waiting for the
+          // router to be idle.
+          if (signal.aborted) return;
+          setTimeout(() => {
+            Object.assign(window.__remixManifest, assetsManifest);
+            window.$RefreshRuntime$.performReactRefresh();
+          }, 0);
         }
       });
       router.revalidate();

packages/remix-react/components.tsx

@@ -1730,9 +1730,11 @@ export const LiveReload =
                       }
                       if (!event.updates || !event.updates.length) return;
                       let updateAccepted = false;
+                      let needsRevalidation = false;
                       for (let update of event.updates) {
                         console.log("[HMR] " + update.reason + " [" + update.id +"]")
                         if (update.revalidate) {
+                          needsRevalidation = true;
                           console.log("[HMR] Revalidating [" + update.id + "]");
                         }
                         let imported = await import(update.url +  '?t=' + event.assetsManifest.hmr.timestamp);
@@ -1748,7 +1750,7 @@ export const LiveReload =
                       }
                       if (event.assetsManifest && window.__hmr__.contexts["remix:manifest"]) {
                         let accepted = window.__hmr__.contexts["remix:manifest"].emit(
-                          event.assetsManifest
+                          { needsRevalidation, assetsManifest: event.assetsManifest }
                         );
                         if (accepted) {
                           console.log("[HMR] Updated accepted by", "remix:manifest");

packages/remix-react/routes.tsx

@@ -104,6 +104,22 @@ export function createServerRoutes(
   });
 }
 
+export function createClientRoutesWithHMRRevalidationOptOut(
+  needsRevalidation: boolean,
+  manifest: RouteManifest<EntryRoute>,
+  routeModulesCache: RouteModules,
+  future: FutureConfig
+) {
+  return createClientRoutes(
+    manifest,
+    routeModulesCache,
+    future,
+    "",
+    groupRoutesByParentId(manifest),
+    needsRevalidation
+  );
+}
+
 export function createClientRoutes(
   manifest: RouteManifest<EntryRoute>,
   routeModulesCache: RouteModules,
@@ -112,7 +128,8 @@ export function createClientRoutes(
   routesByParentId: Record<
     string,
     Omit<EntryRoute, "children">[]
-  > = groupRoutesByParentId(manifest)
+  > = groupRoutesByParentId(manifest),
+  needsRevalidation: boolean | undefined = undefined
 ): DataRouteObject[] {
   return (routesByParentId[parentId] || []).map((route) => {
     let hasErrorBoundary =
@@ -136,7 +153,11 @@ export function createClientRoutes(
       handle: undefined,
       loader: createDataFunction(route, routeModulesCache, false),
       action: createDataFunction(route, routeModulesCache, true),
-      shouldRevalidate: createShouldRevalidate(route, routeModulesCache),
+      shouldRevalidate: createShouldRevalidate(
+        route,
+        routeModulesCache,
+        needsRevalidation
+      ),
     };
     let children = createClientRoutes(
       manifest,
@@ -151,14 +172,29 @@ export function createClientRoutes(
 
 function createShouldRevalidate(
   route: EntryRoute,
-  routeModules: RouteModules
+  routeModules: RouteModules,
+  needsRevalidation: boolean | undefined
 ): ShouldRevalidateFunction {
+  let handledRevalidation = false;
   return function (arg) {
     let module = routeModules[route.id];
     invariant(module, `Expected route module to be loaded for ${route.id}`);
+
     if (module.shouldRevalidate) {
+      if (typeof needsRevalidation === "boolean" && !handledRevalidation) {
+        handledRevalidation = true;
+        return module.shouldRevalidate({
+          ...arg,
+          defaultShouldRevalidate: needsRevalidation,
+        });
+      }
       return module.shouldRevalidate(arg);
     }
+
+    if (typeof needsRevalidation === "boolean" && !handledRevalidation) {
+      handledRevalidation = true;
+      return needsRevalidation;
+    }
     return arg.defaultShouldRevalidate;
   };
 }