Fix fetcher revalidation logic

.changeset/fix-fetcher-revalidation.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Fixed a bug where fetchers were incorrectly attempting to revalidate on search params changes or routing to the same URL (using the same logic for route `loader` revalidations). However, since fetchers have a static href, they should only revalidate on `action` submissions or `router.revalidate` calls.

packages/router/__tests__/router-test.ts

@@ -6249,17 +6249,10 @@ describe("a router", () => {
 
       let fetch = await t.fetch("/parent", "key");
 
-      let B = await fetch.loaders.parent.redirectReturn(
-        "..",
-        undefined,
-        undefined,
-        ["parent"]
-      );
+      await fetch.loaders.parent.redirectReturn("..", undefined, undefined, [
+        "parent",
+      ]);
 
-      // We called fetcher.load('/parent') from the root route, so when we
-      // redirect back to the root it triggers a revalidation of the
-      // fetcher.load('/parent')
-      await B.loaders.parent.resolve("Revalidated");
       // No root loader so redirect lands immediately
       expect(t.router.state).toMatchObject({
         location: {
@@ -6271,7 +6264,7 @@ describe("a router", () => {
       });
       expect(t.router.state.fetchers.get("key")).toMatchObject({
         state: "idle",
-        data: "Revalidated",
+        data: undefined,
       });
     });
 
@@ -9578,7 +9571,7 @@ describe("a router", () => {
         });
       });
 
-      it("revalidates fetchers on searchParams changes", async () => {
+      it("does not revalidate fetchers on searchParams changes", async () => {
         let key = "key";
         let t = setup({
           routes: TASK_ROUTES,
@@ -9601,15 +9594,15 @@ describe("a router", () => {
         let B = await t.navigate("/tasks/1?key=value", undefined, ["index"]);
         await B.loaders.root.resolve("ROOT 2");
         await B.loaders.tasksId.resolve("TASK 2");
-        await B.loaders.index.resolve("FETCH 2");
         expect(t.router.state.loaderData).toMatchObject({
           root: "ROOT 2",
           tasksId: "TASK 2",
         });
         expect(t.router.state.fetchers.get(key)).toMatchObject({
           state: "idle",
-          data: "FETCH 2",
+          data: "FETCH 1",
         });
+        expect(B.loaders.index.stub).not.toHaveBeenCalled();
       });
 
       it("revalidates fetchers on links to the current location", async () => {
@@ -9635,15 +9628,15 @@ describe("a router", () => {
         let B = await t.navigate("/tasks/1", undefined, ["index"]);
         await B.loaders.root.resolve("ROOT 2");
         await B.loaders.tasksId.resolve("TASK 2");
-        await B.loaders.index.resolve("FETCH 2");
         expect(t.router.state.loaderData).toMatchObject({
           root: "ROOT 2",
           tasksId: "TASK 2",
         });
         expect(t.router.state.fetchers.get(key)).toMatchObject({
           state: "idle",
-          data: "FETCH 2",
+          data: "FETCH 1",
         });
+        expect(B.loaders.index.stub).not.toHaveBeenCalled();
       });
 
       it("does not revalidate idle fetchers when a loader navigation is performed", async () => {
@@ -10094,8 +10087,12 @@ describe("a router", () => {
         let A = await t.fetch("/tasks/1", key1);
         await A.loaders.tasksId.resolve("TASKS 1");
 
-        // Loading navigation with query param to trigger revalidations
-        let C = await t.navigate("/tasks?key=value");
+        // Submission navigation to trigger revalidations
+        let C = await t.navigate("/tasks", {
+          formMethod: "post",
+          formData: createFormData({}),
+        });
+        await C.actions.tasks.resolve("TASKS ACTION");
 
         // Fetcher should go back into a loading state
         expect(t.router.state.fetchers.get(key1)).toMatchObject({
@@ -10107,12 +10104,16 @@ describe("a router", () => {
         t.router.deleteFetcher(key1);
         expect(t.router.state.fetchers.get(key1)).toBeUndefined();
 
-        // Resolve navigation loaders
+        // Resolve navigation action/loaders
         await C.loaders.root.resolve("ROOT*");
         await C.loaders.tasks.resolve("TASKS LOADER");
 
         expect(t.router.state).toMatchObject({
           errors: null,
+          navigation: IDLE_NAVIGATION,
+          actionData: {
+            tasks: "TASKS ACTION",
+          },
           loaderData: {
             tasks: "TASKS LOADER",
             root: "ROOT*",

packages/router/router.ts

@@ -814,7 +814,7 @@ export function createRouter(init: RouterInit): Router {
   // Fetchers that triggered data reloads as a result of their actions
   let fetchReloadIds = new Map<string, number>();
 
-  // Fetchers that triggered redirect navigations from their actions
+  // Fetchers that triggered redirect navigations
   let fetchRedirectIds = new Set<string>();
 
   // Most recent href/match for fetcher.load calls for fetchers
@@ -1470,12 +1470,14 @@ export function createRouter(init: RouterInit): Router {
 
     // Short circuit if we have no loaders to run
     if (matchesToLoad.length === 0 && revalidatingFetchers.length === 0) {
+      let updatedFetchers = markFetchRedirectsDone();
       completeNavigation(location, {
         matches,
         loaderData: {},
         // Commit pending error if we're short circuiting
         errors: pendingError || null,
         ...(pendingActionData ? { actionData: pendingActionData } : {}),
+        ...(updatedFetchers ? { fetchers: new Map(state.fetchers) } : {}),
       });
       return { shortCircuited: true };
     }
@@ -1587,15 +1589,15 @@ export function createRouter(init: RouterInit): Router {
       });
     });
 
-    markFetchRedirectsDone();
+    let updatedFetchers = markFetchRedirectsDone();
     let didAbortFetchLoads = abortStaleFetchLoads(pendingNavigationLoadId);
+    let shouldUpdateFetchers =
+      updatedFetchers || didAbortFetchLoads || revalidatingFetchers.length > 0;
 
     return {
       loaderData,
       errors,
-      ...(didAbortFetchLoads || revalidatingFetchers.length > 0
-        ? { fetchers: new Map(state.fetchers) }
-        : {}),
+      ...(shouldUpdateFetchers ? { fetchers: new Map(state.fetchers) } : {}),
     };
   }
 
@@ -1959,7 +1961,7 @@ export function createRouter(init: RouterInit): Router {
         result;
     }
 
-    // We can delete this so long as we weren't aborted by ou our own fetcher
+    // We can delete this so long as we weren't aborted by our our own fetcher
     // re-load which would have put _new_ controller is in fetchControllers
     if (fetchControllers.get(key) === abortController) {
       fetchControllers.delete(key);
@@ -1971,6 +1973,7 @@ export function createRouter(init: RouterInit): Router {
 
     // If the loader threw a redirect Response, start a new REPLACE navigation
     if (isRedirectResult(result)) {
+      fetchRedirectIds.add(key);
       await startRedirectNavigation(state, result);
       return;
     }
@@ -2270,17 +2273,20 @@ export function createRouter(init: RouterInit): Router {
     }
   }
 
-  function markFetchRedirectsDone(): void {
+  function markFetchRedirectsDone(): boolean {
     let doneKeys = [];
+    let updatedFetchers = false;
     for (let key of fetchRedirectIds) {
       let fetcher = state.fetchers.get(key);
       invariant(fetcher, `Expected fetcher: ${key}`);
       if (fetcher.state === "loading") {
         fetchRedirectIds.delete(key);
         doneKeys.push(key);
+        updatedFetchers = true;
       }
     }
     markFetchersDone(doneKeys);
+    return updatedFetchers;
   }
 
   function abortStaleFetchLoads(landedId: number): boolean {
@@ -3133,14 +3139,6 @@ function getMatchesToLoad(
   let currentUrl = history.createURL(state.location);
   let nextUrl = history.createURL(location);
 
-  let defaultShouldRevalidate =
-    // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate
-    isRevalidationRequired ||
-    // Clicked the same link, resubmitted a GET form
-    currentUrl.toString() === nextUrl.toString() ||
-    // Search params affect all loaders
-    currentUrl.search !== nextUrl.search;
-
   // Pick navigation matches that are net-new or qualify for revalidation
   let boundaryId = pendingError ? Object.keys(pendingError)[0] : undefined;
   let boundaryMatches = getLoaderMatchesUntilBoundary(matches, boundaryId);
@@ -3177,7 +3175,12 @@ function getMatchesToLoad(
       ...submission,
       actionResult,
       defaultShouldRevalidate:
-        defaultShouldRevalidate ||
+        // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate
+        isRevalidationRequired ||
+        // Clicked the same link, resubmitted a GET form
+        currentUrl.toString() === nextUrl.toString() ||
+        // Search params affect all loaders
+        currentUrl.search !== nextUrl.search ||
         isNewRouteInstance(currentRouteMatch, nextRouteMatch),
     });
   });
@@ -3231,7 +3234,8 @@ function getMatchesToLoad(
       nextParams: matches[matches.length - 1].params,
       ...submission,
       actionResult,
-      defaultShouldRevalidate,
+      // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate
+      defaultShouldRevalidate: isRevalidationRequired,
     });
     if (shouldRevalidate) {
       revalidatingFetchers.push({