Fix fetcher interruption cleanup logic after fetcher submissions (#11839)

Author: brophdawg11

.changeset/hip-poets-provide.md

@@ -0,0 +1,11 @@
+---
+"@remix-run/router": patch
+---
+
+Fix internal cleanup of interrupted fetchers to avoid invalid revalidations on navigations
+
+- When a `fetcher.load` is interrupted by an `action` submission, we track it internally and force revalidation once the `action` completes
+- We previously only cleared out this internal tracking info on a successful _navigation_ submission
+- Therefore, if the `fetcher.load` was interrupted by a `fetcher.submit`, then we wouldn't remove it from this internal tracking info on successful load (incorrectly)
+- And then on the next navigation it's presence in the internal tracking would automatically trigger execution of the `fetcher.load` again, ignoring any `shouldRevalidate` logic
+- This fix cleans up the internal tracking so it applies to both navigation submission and fetcher submissions

packages/router/__tests__/fetchers-test.ts

@@ -2405,6 +2405,108 @@ describe("fetchers", () => {
       });
     });
 
+    it("cancels in-flight fetcher.loads on fetcher.action submission and forces reload (one time)", async () => {
+      let t = setup({
+        routes: [
+          {
+            id: "root",
+            path: "/",
+            children: [
+              {
+                id: "index",
+                index: true,
+              },
+              {
+                id: "page",
+                path: "page",
+                action: true,
+              },
+              {
+                id: "action",
+                path: "action",
+                action: true,
+              },
+              {
+                id: "fetch",
+                path: "fetch",
+                loader: true,
+                shouldRevalidate: () => false,
+              },
+            ],
+          },
+        ],
+      });
+      expect(t.router.state.navigation).toBe(IDLE_NAVIGATION);
+
+      // Kick off a fetch from the root route
+      let keyA = "a";
+      let A = await t.fetch("/fetch", keyA, "root");
+      expect(t.router.state.fetchers.get(keyA)).toMatchObject({
+        state: "loading",
+        data: undefined,
+      });
+
+      // Interrupt with a fetcher submission which will trigger a forced
+      // revalidation, ignoring shouldRevalidate=false since the prior load
+      // was interrupted
+      let keyB = "b";
+      let B = await t.fetch("/action", keyB, {
+        formMethod: "post",
+        formData: createFormData({}),
+      });
+      t.shimHelper(B.loaders, "fetch", "loader", "fetch");
+      expect(t.router.state.fetchers.get(keyB)).toMatchObject({
+        state: "submitting",
+        data: undefined,
+      });
+
+      // Resolving the first load is a no-op since it would have been aborted
+      await A.loaders.fetch.resolve("A LOADER");
+      expect(t.router.state.fetchers.get(keyA)).toMatchObject({
+        state: "loading",
+        data: undefined,
+      });
+
+      // Resolve the action, kicking off revalidations which will force the
+      // fetcher "a" load to run again even through shouldRevalidate=false
+      await B.actions.action.resolve("B ACTION");
+      expect(t.router.state.fetchers.get(keyB)).toMatchObject({
+        state: "loading",
+        data: "B ACTION",
+      });
+      expect(t.router.state.fetchers.get(keyA)).toMatchObject({
+        state: "loading",
+        data: undefined,
+      });
+
+      // Resolve the second loader kicked off after the action
+      await B.loaders.fetch.resolve("B LOADER");
+      expect(t.router.state.fetchers.get(keyA)).toMatchObject({
+        state: "idle",
+        data: "B LOADER",
+      });
+
+      // submitting to another route
+      let C = await t.navigate("/page", {
+        formMethod: "post",
+        formData: createFormData({}),
+      });
+      t.shimHelper(C.loaders, "navigation", "loader", "fetch");
+      expect(t.router.state.navigation.location?.pathname).toBe("/page");
+
+      // should not trigger a fetcher reload due to shouldRevalidate=false
+      // This fixes a prior bug where we didn't remove the fetcher from
+      // cancelledFetcherLoaders upon the forced revalidation
+      await C.actions.page.resolve("PAGE ACTION");
+      expect(t.router.state.location.pathname).toBe("/page");
+      expect(t.router.state.navigation).toBe(IDLE_NAVIGATION);
+      expect(t.router.state.fetchers.get(keyA)).toMatchObject({
+        state: "idle",
+        data: "B LOADER",
+      });
+      expect(C.loaders.fetch.stub).not.toHaveBeenCalled();
+    });
+
     it("does not cancel pending action navigation on deletion of revalidating fetcher", async () => {
       let t = setup({
         routes: TASK_ROUTES,

packages/router/router.ts

@@ -981,7 +981,7 @@ export function createRouter(init: RouterInit): Router {
 
   // Use this internal array to capture fetcher loads that were cancelled by an
   // action navigation and require revalidation
-  let cancelledFetcherLoads: string[] = [];
+  let cancelledFetcherLoads: Set<string> = new Set();
 
   // AbortControllers for any in-flight fetchers
   let fetchControllers = new Map<string, AbortController>();
@@ -1334,7 +1334,6 @@ export function createRouter(init: RouterInit): Router {
     isUninterruptedRevalidation = false;
     isRevalidationRequired = false;
     cancelledDeferredRoutes = [];
-    cancelledFetcherLoads = [];
   }
 
   // Trigger a navigation event, which can either be a numerical POP or a PUSH
@@ -2867,7 +2866,7 @@ export function createRouter(init: RouterInit): Router {
     // Abort in-flight fetcher loads
     fetchLoadMatches.forEach((_, key) => {
       if (fetchControllers.has(key)) {
-        cancelledFetcherLoads.push(key);
+        cancelledFetcherLoads.add(key);
         abortFetcher(key);
       }
     });
@@ -2931,6 +2930,7 @@ export function createRouter(init: RouterInit): Router {
     fetchReloadIds.delete(key);
     fetchRedirectIds.delete(key);
     deletedFetchers.delete(key);
+    cancelledFetcherLoads.delete(key);
     state.fetchers.delete(key);
   }
 
@@ -4324,7 +4324,7 @@ function getMatchesToLoad(
   skipActionErrorRevalidation: boolean,
   isRevalidationRequired: boolean,
   cancelledDeferredRoutes: string[],
-  cancelledFetcherLoads: string[],
+  cancelledFetcherLoads: Set<string>,
   deletedFetchers: Set<string>,
   fetchLoadMatches: Map<string, FetchLoadMatch>,
   fetchRedirectIds: Set<string>,
@@ -4459,8 +4459,9 @@ function getMatchesToLoad(
     if (fetchRedirectIds.has(key)) {
       // Never trigger a revalidation of an actively redirecting fetcher
       shouldRevalidate = false;
-    } else if (cancelledFetcherLoads.includes(key)) {
-      // Always revalidate if the fetcher was cancelled
+    } else if (cancelledFetcherLoads.has(key)) {
+      // Always mark for revalidation if the fetcher was cancelled
+      cancelledFetcherLoads.delete(key);
       shouldRevalidate = true;
     } else if (
       fetcher &&