[Bug]: check shouldRevalidation before cancel a loader

contributors.yml

@@ -41,6 +41,7 @@
 - bilalk711
 - bobziroll
 - BrianT1414
+- BrianWoolfolk
 - brockross
 - brookslybrand
 - brophdawg11

packages/react-router/__tests__/router/cancel-loader-test.tsx

@@ -0,0 +1,226 @@
+import { createBrowserRouter } from "react-router";
+import { cleanup } from "./utils/data-router-setup";
+import { useEffect } from "react";
+import React from "react";
+import { useFetcher } from "../../lib/dom/lib";
+import { render, screen, waitFor } from "@testing-library/react";
+import { RouterProvider } from "../../lib/components";
+import { sleep } from "./utils/utils";
+
+describe("cancel-loader", () => {
+  afterEach(() => cleanup());
+
+  it("checks ShouldRevalidate before cancelling and calling loader again", async () => {
+    let loaderData = "";
+
+    const Comp = () => {
+      const fetch1 = useFetcher();
+      const fetch2 = useFetcher();
+
+      useEffect(() => {
+        fetch1.load("/loader");
+      }, []);
+
+      useEffect(() => {
+        fetch2.submit(null, { method: "POST", action: "/action" });
+      }, []);
+
+      useEffect(() => {
+        loaderData = fetch1.data;
+      }, [fetch1.data]);
+
+      return <div>something: {fetch1.data}</div>;
+    };
+
+    const testingLoaderFun = jest.fn(async () => {
+      await sleep(1500);
+      return "good info";
+    });
+
+    let router = createBrowserRouter([
+      {
+        path: "/",
+        element: <Comp />,
+      },
+      {
+        path: "/loader",
+        shouldRevalidate: () => false,
+        loader: testingLoaderFun,
+      },
+      {
+        path: "/action",
+        action: () => null,
+      },
+    ]);
+
+    render(<RouterProvider router={router} />);
+    // Don't know how to properly set up tests, sorry
+    // This waits for everything to run and get resolved
+    await sleep(3000);
+    await waitFor(() =>
+      expect(screen.getByText(/something:[\S\s]*good info/)).toBeTruthy()
+    );
+
+    // Fetcher should not get cancelled nor revalidated, so just 1 call
+    expect(testingLoaderFun).toHaveBeenCalledTimes(1);
+    // Verify the loader data to be resolved
+    expect(loaderData).toBe("good info");
+  });
+
+  it("aborts a loader if not explicitly set", async () => {
+    let loaderData = "";
+
+    const Comp = () => {
+      const fetch1 = useFetcher();
+      const fetch2 = useFetcher();
+
+      useEffect(() => {
+        fetch1.load("/loader");
+      }, []);
+
+      useEffect(() => {
+        fetch2.submit(null, { method: "POST", action: "/action" });
+      }, []);
+
+      useEffect(() => {
+        loaderData = fetch1.data;
+      }, [fetch1.data]);
+
+      return <div>something: {fetch1.data}</div>;
+    };
+
+    const testingLoaderFun = jest.fn(async () => {
+      await sleep(1500);
+      return "good info";
+    });
+
+    let router = createBrowserRouter([
+      {
+        path: "/",
+        element: <Comp />,
+      },
+      {
+        path: "/loader",
+        shouldRevalidate: () => true,
+        loader: testingLoaderFun,
+      },
+      {
+        path: "/action",
+        action: () => null,
+      },
+    ]);
+
+    render(<RouterProvider router={router} />);
+
+    await sleep(3000);
+    await waitFor(() =>
+      expect(screen.getByText(/something:[\S\s]*good info/)).toBeTruthy()
+    );
+
+    // Normal behaviour still preserved, call1 cancelled, call2 resolved
+    expect(testingLoaderFun).toHaveBeenCalledTimes(2);
+    expect(loaderData).toBe("good info");
+  });
+
+  it("aborts some loaders and preserves some others", async () => {
+    let loaderData = "";
+
+    const Comp = () => {
+      const fetchAction = useFetcher();
+      const fetchLoader1 = useFetcher();
+      const fetchLoader2 = useFetcher();
+      const fetchLoader3 = useFetcher();
+      const fetchLoader4 = useFetcher();
+      const fetchLoader5 = useFetcher();
+      const fetchLoader6 = useFetcher();
+
+      useEffect(() => {
+        fetchLoader1.load("/loader1");
+        fetchLoader2.load("/loader2");
+        fetchLoader3.load("/loader3");
+        fetchLoader4.load("/loader4");
+        fetchLoader5.load("/loader5");
+        fetchLoader6.load("/loader6");
+      }, []);
+
+      useEffect(() => {
+        fetchAction.submit(null, { method: "POST", action: "/action" });
+      }, []);
+
+      useEffect(() => {
+        loaderData = `${fetchLoader1.data}, ${fetchLoader2.data}, ${fetchLoader3.data}`;
+      }, [fetchLoader1.data, fetchLoader2.data, fetchLoader3.data]);
+
+      return <div>something: {loaderData}</div>;
+    };
+
+    const fnArray: jest.Mock<Promise<string>, [], any>[] = [];
+    for (let i = 1; i <= 6; i++) {
+      const testingLoaderFun = jest.fn(async () => {
+        await sleep(1500);
+
+        return "load-" + i;
+      });
+      fnArray.push(testingLoaderFun);
+    }
+
+    let router = createBrowserRouter([
+      {
+        path: "/",
+        element: <Comp />,
+      },
+      {
+        path: "/loader1",
+        shouldRevalidate: () => false,
+        loader: fnArray[0],
+      },
+      {
+        path: "/loader2",
+        shouldRevalidate: () => false,
+        loader: fnArray[1],
+      },
+      {
+        path: "/loader3",
+        shouldRevalidate: () => false,
+        loader: fnArray[2],
+      },
+      {
+        path: "/loader4",
+        shouldRevalidate: () => true,
+        loader: fnArray[3],
+      },
+      {
+        path: "/loader5",
+        shouldRevalidate: () => true,
+        loader: fnArray[4],
+      },
+      {
+        path: "/loader6",
+        shouldRevalidate: () => true,
+        loader: fnArray[5],
+      },
+      {
+        path: "/action",
+        action: () => null,
+      },
+    ]);
+
+    render(<RouterProvider router={router} />);
+    await sleep(3000);
+
+    await waitFor(() =>
+      expect(
+        screen.getByText(/something:[\S\s]*(load-\d(, )?){1,3}/)
+      ).toBeTruthy()
+    );
+
+    for (let i = 0; i < 6; i++) {
+      expect(fnArray[i]).toHaveBeenCalledTimes(i > 2 ? 2 : 1);
+    }
+    expect(loaderData).toBe(`load-1, load-2, load-3`);
+  });
+
+  // TODO: test with nested loaders
+  // TODO: test redirections inside loader/action
+  // TODO: test errors inside loader/action
+});

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

@@ -2198,7 +2198,12 @@ export function createRouter(init: RouterInit): Router {
     preventScrollReset: boolean,
     submission: Submission
   ) {
-    interruptActiveLoads();
+    // Check if a loader needs revalidation before cancelling it
+    interruptActiveLoadsWithRevalidation(init.history, state);
+
+    // And cancel this one just in case
+    if (fetchControllers.has(key)) cancelledFetcherLoads.add(key);
+    abortFetcher(key);
     fetchLoadMatches.delete(key);
 
     function detectAndHandle405Error(m: AgnosticDataRouteMatch) {
@@ -2868,6 +2873,51 @@ export function createRouter(init: RouterInit): Router {
     });
   }
 
+  function interruptActiveLoadsWithRevalidation(
+    history: History,
+    state: RouterState
+  ) {
+    // Every interruption triggers a revalidation for the others
+    isRevalidationRequired = true;
+
+    // Abort in-flight fetcher loads
+    fetchLoadMatches.forEach((match, key) => {
+      const routesToUse = inFlightDataRoutes || dataRoutes;
+      const fetcherMatches = matchRoutes(routesToUse, match.path, basename);
+
+      if (fetcherMatches) {
+        const fetcherMatch = getTargetMatch(fetcherMatches, match.path);
+
+        let currentUrl = history.createURL(state.location);
+        let nextUrl = history.createURL(location);
+        let matches =
+          state.navigation.state !== "idle"
+            ? matchRoutes(routesToUse, state.navigation.location, basename)
+            : state.matches;
+
+        // Before cancelling the fetcher, ask its revalidator
+        if (
+          matches &&
+          !shouldRevalidateLoader(fetcherMatch, {
+            currentUrl,
+            currentParams: state.matches[state.matches.length - 1].params,
+            nextUrl,
+            nextParams: matches[matches.length - 1].params,
+            defaultShouldRevalidate: isRevalidationRequired,
+          })
+        ) {
+          // If loader does not need revalidation we let it async
+          return;
+        }
+      }
+
+      if (fetchControllers.has(key)) {
+        cancelledFetcherLoads.add(key);
+      }
+      abortFetcher(key);
+    });
+  }
+
   function updateFetcherState(
     key: string,
     fetcher: Fetcher,
@@ -4362,6 +4412,18 @@ function getMatchesToLoad(
       // revalidation, it would just be a brand new load if an explicit
       // revalidation is required
       shouldRevalidate = isRevalidationRequired;
+
+      // If we first see for cancelled fetchers, and then for pending ones,
+      // it seems that no fetcher would ever got in here because methods like
+      // handleFetcherAction first aborts every fetcher there is, so every of
+      // them would fall in the previous `if` block (except for the ones called
+      // after the action initial call).
+      if (fetcher.state === "loading") {
+        // Naturally, if a loader is still in process, we want it to finish because
+        // it most likely got caught by `interruptActiveLoadsWithRevalidation` and
+        // so it already told us to not revalidate it.
+        return;
+      }
     } else {
       // Otherwise fall back on any user-defined shouldRevalidate, defaulting
       // to explicit revalidations only