fix: preserve loaderData for non-revalidating loaders (#8973)

* fix: preserve loaderData for non-revalidating loaders

* add changeset

* chore: refactor implementation to leverage mergeLoaderData

.changeset/unlucky-cows-rhyme.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+fix: preserve loader data for loaders that opted out of revalidation (#8973)

packages/router/__tests__/router-test.ts

@@ -1601,6 +1601,120 @@ describe("a router", () => {
 
       router.dispose();
     });
+
+    it("preserves non-revalidated loaderData on navigations", async () => {
+      let count = 0;
+      let history = createMemoryHistory();
+      let router = createRouter({
+        history,
+        routes: [
+          {
+            path: "",
+            id: "root",
+            loader: () => `ROOT ${++count}`,
+            element: {},
+            children: [
+              {
+                path: "/",
+                id: "index",
+                loader: (args) => "SHOULD NOT GET CALLED",
+                shouldRevalidate: () => false,
+              },
+            ],
+          },
+        ],
+        hydrationData: {
+          loaderData: {
+            root: "ROOT 0",
+            index: "INDEX",
+          },
+        },
+      });
+      router.initialize();
+      await tick();
+
+      // Navigating to the same link would normally cause all loaders to re-run
+      router.navigate("/");
+      await tick();
+      expect(router.state.loaderData).toEqual({
+        root: "ROOT 1",
+        index: "INDEX",
+      });
+
+      router.navigate("/");
+      await tick();
+      expect(router.state.loaderData).toEqual({
+        root: "ROOT 2",
+        index: "INDEX",
+      });
+
+      router.dispose();
+    });
+
+    it("preserves non-revalidated loaderData on fetches", async () => {
+      let count = 0;
+      let history = createMemoryHistory();
+      let router = createRouter({
+        history,
+        routes: [
+          {
+            path: "",
+            id: "root",
+            element: {},
+            children: [
+              {
+                path: "/",
+                id: "index",
+                loader: () => "SHOULD NOT GET CALLED",
+                shouldRevalidate: () => false,
+              },
+              {
+                path: "/fetch",
+                id: "fetch",
+                action: () => `FETCH ${++count}`,
+              },
+            ],
+          },
+        ],
+        hydrationData: {
+          loaderData: {
+            index: "INDEX",
+          },
+        },
+      });
+      router.initialize();
+      await tick();
+
+      let key = "key";
+
+      router.fetch(key, "root", "/fetch", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+      });
+      await tick();
+      expect(router.state.fetchers.get(key)).toMatchObject({
+        state: "idle",
+        data: "FETCH 1",
+      });
+      expect(router.state.loaderData).toMatchObject({
+        index: "INDEX",
+      });
+
+      router.fetch(key, "root", "/fetch", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+      });
+      await tick();
+      expect(router.state.fetchers.get(key)).toMatchObject({
+        state: "idle",
+        data: "FETCH 2",
+      });
+      expect(router.state.loaderData).toMatchObject({
+        index: "INDEX",
+      });
+
+      router.dispose();
+    });
   });
 
   describe("no route match", () => {
@@ -1633,6 +1747,57 @@ describe("a router", () => {
         },
       ]);
     });
+
+    it("clears prior loader/action data", async () => {
+      let t = initializeTmTest();
+      expect(t.router.state.loaderData).toEqual({
+        root: "ROOT",
+        index: "INDEX",
+      });
+
+      let A = await t.navigate("/foo", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+      });
+      await A.actions.foo.resolve("ACTION");
+      await A.loaders.root.resolve("ROOT*");
+      await A.loaders.foo.resolve("LOADER");
+      expect(t.router.state.actionData).toEqual({
+        foo: "ACTION",
+      });
+      expect(t.router.state.loaderData).toEqual({
+        root: "ROOT*",
+        foo: "LOADER",
+      });
+
+      t.navigate("/not-found");
+      expect(t.router.state.actionData).toBe(null);
+      expect(t.router.state.loaderData).toEqual({
+        root: "ROOT*",
+      });
+      expect(t.router.state.errors).toEqual({
+        root: {
+          status: 404,
+          statusText: "Not Found",
+          data: null,
+        },
+      });
+      expect(t.router.state.matches).toMatchObject([
+        {
+          params: {},
+          pathname: "",
+          route: {
+            errorElement: true,
+            children: expect.any(Array),
+            element: {},
+            id: "root",
+            loader: expect.any(Function),
+            module: "",
+            path: "",
+          },
+        },
+      ]);
+    });
   });
 
   describe("errors on navigation", () => {

packages/router/router.ts

@@ -597,19 +597,29 @@ export function createRouter(init: RouterInit): Router {
       state.navigation.formMethod != null &&
       state.navigation.state === "loading";
 
+    // Always preserve any existing loaderData from re-used routes
+    let newLoaderData = newState.loaderData
+      ? {
+          loaderData: mergeLoaderData(
+            state.loaderData,
+            newState.loaderData,
+            newState.matches || []
+          ),
+        }
+      : {};
+
     updateState({
       // Clear existing actionData on any completed navigation beyond the original
       // action, unless we're currently finishing the loading/actionReload state.
       // Do this prior to spreading in newState in case we got back to back actions
       ...(isActionReload ? {} : { actionData: null }),
       ...newState,
+      ...newLoaderData,
       historyAction,
       location,
       initialized: true,
       navigation: IDLE_NAVIGATION,
       revalidation: "idle",
-      // Always preserve any existing loaderData from re-used routes
-      loaderData: mergeLoaderData(state, newState),
       // Don't restore on submission navigations
       restoreScrollPosition: state.navigation.formData
         ? false
@@ -749,6 +759,7 @@ export function createRouter(init: RouterInit): Router {
       } = getNotFoundMatches(dataRoutes);
       completeNavigation(historyAction, location, {
         matches: notFoundMatches,
+        loaderData: {},
         errors: {
           [route.id]: error,
         },
@@ -1292,10 +1303,12 @@ export function createRouter(init: RouterInit): Router {
         fetchers: new Map(state.fetchers),
       });
     } else {
-      // otherwise just update with the fetcher data
+      // otherwise just update with the fetcher data, preserving any existing
+      // loaderData for loaders that did not need to reload.  We have to
+      // manually merge here since we aren't going through completeNavigation
       updateState({
         errors,
-        loaderData,
+        loaderData: mergeLoaderData(state.loaderData, loaderData, matches),
         ...(didAbortFetchLoads ? { fetchers: new Map(state.fetchers) } : {}),
       });
       isRevalidationRequired = false;
@@ -1937,26 +1950,18 @@ function processLoaderData(
 }
 
 function mergeLoaderData(
-  state: RouterState,
-  newState: Partial<RouterState>
+  loaderData: RouteData,
+  newLoaderData: RouteData,
+  matches: DataRouteMatch[]
 ): RouteData {
-  // Identify active routes that have current loaderData and didn't receive new
-  // loaderData
-  let reusedRoutesWithData = (newState.matches || state.matches).filter(
-    (match) =>
-      state.loaderData[match.route.id] !== undefined &&
-      newState.loaderData?.[match.route.id] === undefined
-  );
-  return {
-    ...newState.loaderData,
-    ...reusedRoutesWithData.reduce(
-      (acc, match) =>
-        Object.assign(acc, {
-          [match.route.id]: state.loaderData[match.route.id],
-        }),
-      {}
-    ),
-  };
+  let mergedLoaderData = { ...newLoaderData };
+  matches.forEach((match) => {
+    let id = match.route.id;
+    if (newLoaderData[id] === undefined && loaderData[id] !== undefined) {
+      mergedLoaderData[id] = loaderData[id];
+    }
+  });
+  return mergedLoaderData;
 }
 
 // Find the nearest error boundary, looking upwards from the leaf route (or the