From 91e730c92d932357a2e9338c8318948ce0311e75 Mon Sep 17 00:00:00 2001 From: Brian Woolfolk Date: Wed, 9 Oct 2024 16:04:04 -0700 Subject: [PATCH 1/2] interruptActiveLoadsWithRevalidation inital implementation --- .../__tests__/router/cancel-loader-test.tsx | 226 ++++++++++++++++++ packages/react-router/lib/router/router.ts | 64 ++++- 2 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 packages/react-router/__tests__/router/cancel-loader-test.tsx diff --git a/packages/react-router/__tests__/router/cancel-loader-test.tsx b/packages/react-router/__tests__/router/cancel-loader-test.tsx new file mode 100644 index 0000000000..05577ddafb --- /dev/null +++ b/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
something: {fetch1.data}
; + }; + + const testingLoaderFun = jest.fn(async () => { + await sleep(1500); + return "good info"; + }); + + let router = createBrowserRouter([ + { + path: "/", + element: , + }, + { + path: "/loader", + shouldRevalidate: () => false, + loader: testingLoaderFun, + }, + { + path: "/action", + action: () => null, + }, + ]); + + render(); + // 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
something: {fetch1.data}
; + }; + + const testingLoaderFun = jest.fn(async () => { + await sleep(1500); + return "good info"; + }); + + let router = createBrowserRouter([ + { + path: "/", + element: , + }, + { + path: "/loader", + shouldRevalidate: () => true, + loader: testingLoaderFun, + }, + { + path: "/action", + action: () => null, + }, + ]); + + render(); + + 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
something: {loaderData}
; + }; + + const fnArray: jest.Mock, [], 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: , + }, + { + 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(); + 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 +}); diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index a7eedb871c..9ae4c17228 100644 --- a/packages/react-router/lib/router/router.ts +++ b/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 From 032ba9d65d11fd7cdcbabdcb173ab360da557edd Mon Sep 17 00:00:00 2001 From: Brian Woolfolk Date: Wed, 9 Oct 2024 16:39:23 -0700 Subject: [PATCH 2/2] CLA signed --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index cff1a1df41..b384c36527 100644 --- a/contributors.yml +++ b/contributors.yml @@ -41,6 +41,7 @@ - bilalk711 - bobziroll - BrianT1414 +- BrianWoolfolk - brockross - brookslybrand - brophdawg11