Fix fetcher data cleanup (#12681)

Author: brophdawg11

.changeset/beige-ants-pay.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Fix issue with fetcher data cleanup in the data layer on fetcher unmount

packages/react-router/__tests__/dom/data-browser-router-test.tsx

@@ -5768,6 +5768,74 @@ function testDomRouter(
             expect(container.innerHTML).toMatch(/(:r[0-9]?[a-z]:),my-key/)
           );
         });
+
+        it("cleans up keyed fetcher data on unmount", 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"));
+
+          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", () => {

packages/react-router/__tests__/router/fetchers-test.ts

@@ -354,6 +354,43 @@ describe("fetchers", () => {
       expect(t.router.state.fetchers.size).toBe(0);
     });
 
+    it("fetchers removed from data layer upon unmount", async () => {
+      let t = initializeTest();
+
+      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();
 

packages/react-router/lib/components.tsx

@@ -217,12 +217,12 @@ export function RouterProvider({
       newState: RouterState,
       { deletedFetchers, flushSync, 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));
 
       warnOnce(
         flushSync === false || reactDomFlushSyncImpl != null,

packages/react-router/lib/router/router.ts

@@ -1135,7 +1135,7 @@ export function createRouter(init: RouterInit): Router {
     // care about in-flight fetchers
     // - If it's been unmounted then we can completely delete it
     // - If it's still mounted we can remove it from `state.fetchers`, but we
-    //   need to keep it around in thing like `fetchLoadMatches`, etc. since
+    //   need to keep it around in things like `fetchLoadMatches`, etc. since
     //   it may be called again
     let unmountedFetchers: string[] = [];
     let mountedFetchers: string[] = [];
@@ -1150,6 +1150,15 @@ export function createRouter(init: RouterInit): Router {
       }
     });
 
+    // Delete any other `idle` fetchers unmounted in the UI that were previously
+    // removed from state.fetchers.  Check `fetchControllers` in case this
+    // fetcher is actively revalidating and we want to let that finish
+    fetchersQueuedForDeletion.forEach((key) => {
+      if (!state.fetchers.has(key) && !fetchControllers.has(key)) {
+        unmountedFetchers.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