Allow persisted fetchers unmounted during submit to revalidate (#11102)

* Allow persisted fetchers unmounted during submit to revalidate * Bump bundle
remix-run · Dec 7, 2023 · 54ec39e · 54ec39e
1 parent a76ee41
commit 54ec39e
Show file tree

Hide file tree

Showing 5 changed files with 171 additions and 27 deletions.
diff --git a/.changeset/silver-teachers-doubt.md b/.changeset/silver-teachers-doubt.md
@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Fix bug preventing revalidation from occuring for persisted fetchers unmounted during the `submitting` phase
diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx
@@ -5712,10 +5712,10 @@ function testDomRouter(
             await waitFor(() => screen.getByText("Page (1)"));
             expect(getHtml(container)).toMatch("Num fetchers: 1");
 
-            // Resolve after the navigation and revalidation
+            // Resolve action after the navigation and trigger revalidation
             dfd.resolve("FETCH");
             await waitFor(() => screen.getByText("Num fetchers: 0"));
-            expect(getHtml(container)).toMatch("Page (1)");
+            expect(getHtml(container)).toMatch("Page (2)");
           });
 
           it("submitting fetchers w/revalidations are cleaned up on completion (remounted)", async () => {

diff --git a/packages/router/__tests__/fetchers-test.ts b/packages/router/__tests__/fetchers-test.ts
@@ -1,5 +1,5 @@
 /* eslint-disable jest/valid-title */
-import type { HydrationState } from "../index";
+import type { FutureConfig, HydrationState } from "../index";
 import {
   createMemoryHistory,
   createRouter,
@@ -21,6 +21,7 @@ import { createFormData, tick } from "./utils/utils";
 function initializeTest(init?: {
   url?: string;
   hydrationData?: HydrationState;
+  future?: Partial<FutureConfig>;
 }) {
   return setup({
     routes: [
@@ -66,6 +67,7 @@ function initializeTest(init?: {
     hydrationData: init?.hydrationData || {
       loaderData: { root: "ROOT", index: "INDEX" },
     },
+    future: init?.future,
     ...(init?.url ? { initialEntries: [init.url] } : {}),
   });
 }
@@ -82,6 +84,7 @@ describe("fetchers", () => {
     // @ts-ignore
     console.warn.mockReset();
   });
+
   describe("fetcher states", () => {
     it("unabstracted loader fetch", async () => {
       let dfd = createDeferred();
@@ -338,6 +341,134 @@ describe("fetchers", () => {
     });
   });
 
+  describe("fetcher removal (w/v7_fetcherPersist)", () => {
+    it("loading fetchers persist until completion", async () => {
+      let t = initializeTest({ future: { v7_fetcherPersist: true } });
+
+      let key = "key";
+      t.router.getFetcher(key); // mount
+      expect(t.router.state.fetchers.size).toBe(0);
+
+      let A = await t.fetch("/foo", key);
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      t.router.deleteFetcher(key); // unmount
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      // Cleaned up on completion
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.fetchers.size).toBe(0);
+    });
+
+    it("submitting fetchers persist until completion when removed during submitting phase", async () => {
+      let t = initializeTest({ future: { v7_fetcherPersist: true } });
+
+      let key = "key";
+      expect(t.router.state.fetchers.size).toBe(0);
+
+      t.router.getFetcher(key); // mount
+      let A = await t.fetch("/foo", key, {
+        formMethod: "post",
+        formData: createFormData({}),
+      });
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("submitting");
+
+      t.router.deleteFetcher(key); // unmount
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("submitting");
+
+      await A.actions.foo.resolve("FOO");
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      // Cleaned up on completion
+      await A.loaders.root.resolve("ROOT*");
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      await A.loaders.index.resolve("INDEX*");
+      expect(t.router.state.fetchers.size).toBe(0);
+    });
+
+    it("submitting fetchers persist until completion when removed during loading phase", async () => {
+      let t = initializeTest({ future: { v7_fetcherPersist: true } });
+
+      let key = "key";
+      t.router.getFetcher(key); // mount
+      expect(t.router.state.fetchers.size).toBe(0);
+
+      let A = await t.fetch("/foo", key, {
+        formMethod: "post",
+        formData: createFormData({}),
+      });
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("submitting");
+
+      await A.actions.foo.resolve("FOO");
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      t.router.deleteFetcher(key); // unmount
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      // Cleaned up on completion
+      await A.loaders.root.resolve("ROOT*");
+      expect(t.router.state.fetchers.size).toBe(1);
+      expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
+
+      await A.loaders.index.resolve("INDEX*");
+      expect(t.router.state.fetchers.size).toBe(0);
+    });
+
+    it("unmounted fetcher.load errors/redirects should not be processed", async () => {
+      let t = initializeTest({ future: { v7_fetcherPersist: true } });
+
+      t.router.getFetcher("a"); // mount
+      let A = await t.fetch("/foo", "a");
+      t.router.deleteFetcher("a"); //unmount
+      await A.loaders.foo.reject("ERROR");
+      expect(t.router.state.fetchers.size).toBe(0);
+      expect(t.router.state.errors).toBe(null);
+
+      t.router.getFetcher("b"); // mount
+      let B = await t.fetch("/bar", "b");
+      t.router.deleteFetcher("b"); //unmount
+      await B.loaders.bar.redirect("/baz");
+      expect(t.router.state.fetchers.size).toBe(0);
+      expect(t.router.state.navigation).toBe(IDLE_NAVIGATION);
+      expect(t.router.state.location.pathname).toBe("/");
+    });
+
+    it("unmounted fetcher.submit errors/redirects should not be processed", async () => {
+      let t = initializeTest({ future: { v7_fetcherPersist: true } });
+
+      t.router.getFetcher("a"); // mount
+      let A = await t.fetch("/foo", "a", {
+        formMethod: "post",
+        formData: createFormData({}),
+      });
+      t.router.deleteFetcher("a"); //unmount
+      await A.actions.foo.reject("ERROR");
+      expect(t.router.state.fetchers.size).toBe(0);
+      expect(t.router.state.errors).toBe(null);
+
+      t.router.getFetcher("b"); // mount
+      let B = await t.fetch("/bar", "b", {
+        formMethod: "post",
+        formData: createFormData({}),
+      });
+      t.router.deleteFetcher("b"); //unmount
+      await B.actions.bar.redirect("/baz");
+      expect(t.router.state.fetchers.size).toBe(0);
+      expect(t.router.state.navigation).toBe(IDLE_NAVIGATION);
+      expect(t.router.state.location.pathname).toBe("/");
+    });
+  });
+
   describe("fetcher error states (4xx Response)", () => {
     it("loader fetch", async () => {
       let t = initializeTest();

diff --git a/packages/router/__tests__/utils/data-router-setup.ts b/packages/router/__tests__/utils/data-router-setup.ts
@@ -142,7 +142,7 @@ type SetupOpts = {
   initialEntries?: InitialEntry[];
   initialIndex?: number;
   hydrationData?: HydrationState;
-  future?: FutureConfig;
+  future?: Partial<FutureConfig>;
 };
 
 // We use a slightly modified version of createDeferred here that includes the

diff --git a/packages/router/router.ts b/packages/router/router.ts
@@ -1981,33 +1981,39 @@ export function createRouter(init: RouterInit): Router {
       return;
     }
 
-    if (deletedFetchers.has(key)) {
-      updateFetcherState(key, getDoneFetcher(undefined));
-      return;
-    }
-
-    if (isRedirectResult(actionResult)) {
-      fetchControllers.delete(key);
-      if (pendingNavigationLoadId > originatingLoadId) {
-        // A new navigation was kicked off after our action started, so that
-        // should take precedence over this redirect navigation.  We already
-        // set isRevalidationRequired so all loaders for the new route should
-        // fire unless opted out via shouldRevalidate
+    // When using v7_fetcherPersist, we don't want errors bubbling up to the UI
+    // or redirects processed for unmounted fetchers so we just revert them to
+    // idle
+    if (future.v7_fetcherPersist && deletedFetchers.has(key)) {
+      if (isRedirectResult(actionResult) || isErrorResult(actionResult)) {
         updateFetcherState(key, getDoneFetcher(undefined));
         return;
-      } else {
-        fetchRedirectIds.add(key);
-        updateFetcherState(key, getLoadingFetcher(submission));
-        return startRedirectNavigation(state, actionResult, {
-          fetcherSubmission: submission,
-        });
       }
-    }
+      // Let SuccessResult's fall through for revalidation
+    } else {
+      if (isRedirectResult(actionResult)) {
+        fetchControllers.delete(key);
+        if (pendingNavigationLoadId > originatingLoadId) {
+          // A new navigation was kicked off after our action started, so that
+          // should take precedence over this redirect navigation.  We already
+          // set isRevalidationRequired so all loaders for the new route should
+          // fire unless opted out via shouldRevalidate
+          updateFetcherState(key, getDoneFetcher(undefined));
+          return;
+        } else {
+          fetchRedirectIds.add(key);
+          updateFetcherState(key, getLoadingFetcher(submission));
+          return startRedirectNavigation(state, actionResult, {
+            fetcherSubmission: submission,
+          });
+        }
+      }
 
-    // Process any non-redirect errors thrown
-    if (isErrorResult(actionResult)) {
-      setFetcherError(key, routeId, actionResult.error);
-      return;
+      // Process any non-redirect errors thrown
+      if (isErrorResult(actionResult)) {
+        setFetcherError(key, routeId, actionResult.error);
+        return;
+      }
     }
 
     if (isDeferredResult(actionResult)) {
@@ -2237,6 +2243,8 @@ export function createRouter(init: RouterInit): Router {
       return;
     }
 
+    // We don't want errors bubbling up or redirects followed for unmounted
+    // fetchers, so short circuit here if it was removed from the UI
     if (deletedFetchers.has(key)) {
       updateFetcherState(key, getDoneFetcher(undefined));
       return;