Fix fetcher data persistence/cleanup issues (#12674)

remix-run · Jan 3, 2025 · e88adb6 · e88adb6
1 parent 97fed11
commit e88adb6
Show file tree

Hide file tree

Showing 5 changed files with 220 additions and 17 deletions.
diff --git a/.changeset/beige-ants-pay.md b/.changeset/beige-ants-pay.md
@@ -0,0 +1,6 @@
+---
+"@remix-run/router": patch
+---
+
+- Fix issue with fetcher data cleanup in the data layer on fetcher unmount
+- Fix behavior of manual fetcher keys when not opted into `future.v7_fetcherPersist`
diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx
@@ -5649,6 +5649,146 @@ function testDomRouter(
             expect(container.innerHTML).toMatch(/(:r[0-9]?[a-z]:),my-key/)
           );
         });
+
+        it("cleans up keyed fetcher data on unmount (w/o fetcherPersist", async () => {
+          let count = 0;
+          let router = createTestRouter(
+            [
+              {
+                path: "/",
+                loader() {
+                  return ++count;
+                },
+                Component() {
+                  let [shown, setShown] = React.useState(false);
+                  return (
+                    <div>
+                      <button onClick={() => setShown(!shown)}>
+                        {shown ? "Unmount" : "Mount"}
+                      </button>
+                      {shown ? <FetcherComponent /> : null}
+                    </div>
+                  );
+                },
+                ErrorBoundary() {
+                  let error = useRouteError();
+                  return <pre>{JSON.stringify(error)}</pre>;
+                },
+              },
+            ],
+            {
+              window: getWindow("/"),
+            }
+          );
+
+          render(<RouterProvider router={router} />);
+
+          function FetcherComponent() {
+            let fetcher = useFetcher({ key: "shared" });
+            return (
+              <div>
+                <p>{`Fetcher state:${fetcher.state}`}</p>
+                {fetcher.data != null ? (
+                  <p data-testid="value">{fetcher.data}</p>
+                ) : null}
+                <button onClick={() => fetcher.load(".")}>Fetch</button>
+              </div>
+            );
+          }
+
+          await waitFor(() => screen.getByText("Mount"));
+
+          fireEvent.click(screen.getByText("Mount"));
+          await waitFor(() => screen.getByText("Fetcher state:idle"));
+
+          fireEvent.click(screen.getByText("Fetch"));
+          await waitFor(() => screen.getByTestId("value"));
+          let value = screen.getByTestId("value").innerHTML;
+
+          fireEvent.click(screen.getByText("Unmount"));
+          await waitFor(() => screen.getByText("Mount"));
+
+          debugger;
+          fireEvent.click(screen.getByText("Mount"));
+          await waitFor(() => screen.getByText("Fetcher state:idle"));
+          expect(screen.queryByTestId("value")).toBe(null);
+
+          fireEvent.click(screen.getByText("Fetch"));
+          await waitFor(() => screen.getByTestId("value"));
+          let value2 = screen.getByTestId("value").innerHTML;
+          expect(value2).not.toBe(value);
+        });
+
+        it("cleans up keyed fetcher data on unmount (w/fetcherPersist", async () => {
+          let count = 0;
+          let router = createTestRouter(
+            [
+              {
+                path: "/",
+                loader() {
+                  return ++count;
+                },
+                Component() {
+                  let [shown, setShown] = React.useState(false);
+                  return (
+                    <div>
+                      <button onClick={() => setShown(!shown)}>
+                        {shown ? "Unmount" : "Mount"}
+                      </button>
+                      {shown ? <FetcherComponent /> : null}
+                    </div>
+                  );
+                },
+                ErrorBoundary() {
+                  let error = useRouteError();
+                  return <pre>{JSON.stringify(error)}</pre>;
+                },
+              },
+            ],
+            {
+              window: getWindow("/"),
+              future: {
+                v7_fetcherPersist: true,
+              },
+            }
+          );
+
+          render(<RouterProvider router={router} />);
+
+          function FetcherComponent() {
+            let fetcher = useFetcher({ key: "shared" });
+            return (
+              <div>
+                <p>{`Fetcher state:${fetcher.state}`}</p>
+                {fetcher.data != null ? (
+                  <p data-testid="value">{fetcher.data}</p>
+                ) : null}
+                <button onClick={() => fetcher.load(".")}>Fetch</button>
+              </div>
+            );
+          }
+
+          await waitFor(() => screen.getByText("Mount"));
+
+          fireEvent.click(screen.getByText("Mount"));
+          await waitFor(() => screen.getByText("Fetcher state:idle"));
+
+          fireEvent.click(screen.getByText("Fetch"));
+          await waitFor(() => screen.getByTestId("value"));
+          let value = screen.getByTestId("value").innerHTML;
+
+          fireEvent.click(screen.getByText("Unmount"));
+          await waitFor(() => screen.getByText("Mount"));
+
+          fireEvent.click(screen.getByText("Mount"));
+          await waitFor(() => screen.getByText("Fetcher state:idle"));
+          expect(screen.queryByTestId("value")).toBe(null);
+
+          fireEvent.click(screen.getByText("Fetch"));
+          await waitFor(() => screen.getByTestId("value"));
+          let value2 = screen.getByTestId("value").innerHTML;
+          expect(value2).not.toBe(value);
+        });
       });
 
       describe("fetcher persistence", () => {

diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx
@@ -525,12 +525,12 @@ export function RouterProvider({
         viewTransitionOpts: viewTransitionOpts,
       }
     ) => {
-      deletedFetchers.forEach((key) => fetcherData.current.delete(key));
       newState.fetchers.forEach((fetcher, key) => {
         if (fetcher.data !== undefined) {
           fetcherData.current.set(key, fetcher.data);
         }
       });
+      deletedFetchers.forEach((key) => fetcherData.current.delete(key));
 
       let isViewTransitionUnavailable =
         router.window == null ||

diff --git a/packages/router/__tests__/fetchers-test.ts b/packages/router/__tests__/fetchers-test.ts
@@ -362,6 +362,43 @@ describe("fetchers", () => {
       expect(t.router.state.fetchers.size).toBe(0);
     });
 
+    it("fetchers removed from data layer upon unmount", async () => {
+      let t = initializeTest({ future: { v7_fetcherPersist: true } });
+
+      let subscriber = jest.fn();
+      t.router.subscribe(subscriber);
+
+      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");
+      expect(subscriber.mock.calls.length).toBe(1);
+      expect(subscriber.mock.calls[0][0].fetchers.get("key").state).toBe(
+        "loading"
+      );
+      subscriber.mockReset();
+
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.fetchers.size).toBe(0);
+      expect(subscriber.mock.calls.length).toBe(1);
+      // Fetcher removed from router state upon return to idle
+      expect(subscriber.mock.calls[0][0].fetchers.size).toBe(0);
+      // But still mounted so not deleted from data layer yet
+      expect(subscriber.mock.calls[0][1].deletedFetchers.length).toBe(0);
+      subscriber.mockReset();
+
+      t.router.deleteFetcher(key); // unmount
+      expect(t.router.state.fetchers.size).toBe(0);
+      expect(subscriber.mock.calls.length).toBe(1);
+      expect(subscriber.mock.calls[0][0].fetchers.size).toBe(0);
+      // Unmounted so can be deleted from data layer
+      expect(subscriber.mock.calls[0][1].deletedFetchers).toEqual(["key"]);
+      subscriber.mockReset();
+    });
+
     it("submitting fetchers persist until completion when removed during submitting phase", async () => {
       let t = initializeTest({ future: { v7_fetcherPersist: true } });
 

diff --git a/packages/router/router.ts b/packages/router/router.ts
@@ -1162,6 +1162,14 @@ export function createRouter(init: RouterInit): Router {
       });
     }
 
+    // Remove any lingering deleted fetchers that have already been removed
+    // from state.fetchers
+    deletedFetchers.forEach((key) => {
+      if (!state.fetchers.has(key) && !fetchControllers.has(key)) {
+        deletedFetchersKeys.push(key);
+      }
+    });
+
     // Iterate over a local copy so that if flushSync is used and we end up
     // removing and adding a new subscriber due to the useCallback dependencies,
     // we don't get ourselves into a loop calling the new subscriber immediately
@@ -1177,6 +1185,10 @@ export function createRouter(init: RouterInit): Router {
     if (future.v7_fetcherPersist) {
       completedFetchers.forEach((key) => state.fetchers.delete(key));
       deletedFetchersKeys.forEach((key) => deleteFetcher(key));
+    } else {
+      // We already called deleteFetcher() on these, can remove them from this
+      // Set now that we've handed the keys off to the data layer
+      deletedFetchersKeys.forEach((key) => deletedFetchers.delete(key));
     }
   }
 
@@ -2942,13 +2954,11 @@ export function createRouter(init: RouterInit): Router {
   }
 
   function getFetcher<TData = any>(key: string): Fetcher<TData> {
-    if (future.v7_fetcherPersist) {
-      activeFetchers.set(key, (activeFetchers.get(key) || 0) + 1);
-      // If this fetcher was previously marked for deletion, unmark it since we
-      // have a new instance
-      if (deletedFetchers.has(key)) {
-        deletedFetchers.delete(key);
-      }
+    activeFetchers.set(key, (activeFetchers.get(key) || 0) + 1);
+    // If this fetcher was previously marked for deletion, unmark it since we
+    // have a new instance
+    if (deletedFetchers.has(key)) {
+      deletedFetchers.delete(key);
     }
     return state.fetchers.get(key) || IDLE_FETCHER;
   }
@@ -2967,23 +2977,33 @@ export function createRouter(init: RouterInit): Router {
     fetchLoadMatches.delete(key);
     fetchReloadIds.delete(key);
     fetchRedirectIds.delete(key);
-    deletedFetchers.delete(key);
+
+    // If we opted into the flag we can clear this now since we're calling
+    // deleteFetcher() at the end of updateState() and we've already handed the
+    // deleted fetcher keys off to the data layer.
+    // If not, we're eagerly calling deleteFetcher() and we need to keep this
+    // Set populated until the next updateState call, and we'll clear
+    // `deletedFetchers` then
+    if (future.v7_fetcherPersist) {
+      deletedFetchers.delete(key);
+    }
+
     cancelledFetcherLoads.delete(key);
     state.fetchers.delete(key);
   }
 
   function deleteFetcherAndUpdateState(key: string): void {
-    if (future.v7_fetcherPersist) {
-      let count = (activeFetchers.get(key) || 0) - 1;
-      if (count <= 0) {
-        activeFetchers.delete(key);
-        deletedFetchers.add(key);
-      } else {
-        activeFetchers.set(key, count);
+    let count = (activeFetchers.get(key) || 0) - 1;
+    if (count <= 0) {
+      activeFetchers.delete(key);
+      deletedFetchers.add(key);
+      if (!future.v7_fetcherPersist) {
+        deleteFetcher(key);
       }
     } else {
-      deleteFetcher(key);
+      activeFetchers.set(key, count);
     }
+
     updateState({ fetchers: new Map(state.fetchers) });
   }