From 718ec1faf6003858b8092e36417b8df15ae8de99 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 21 Jun 2023 10:02:30 -0400 Subject: [PATCH] Avoid calling shouldRevalidate on interrupted initial load fetchers (#10623) * Avoid calling shouldRevalidate on interrupted initial load fetchers * Bump bundle --- .changeset/skip-fetcher-revalidate.md | 5 ++ package.json | 2 +- packages/router/__tests__/router-test.ts | 56 +++++++++++++++++++++ packages/router/router.ts | 63 ++++++++++++++---------- 4 files changed, 100 insertions(+), 26 deletions(-) create mode 100644 .changeset/skip-fetcher-revalidate.md diff --git a/.changeset/skip-fetcher-revalidate.md b/.changeset/skip-fetcher-revalidate.md new file mode 100644 index 0000000000..128a2c4d39 --- /dev/null +++ b/.changeset/skip-fetcher-revalidate.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Avoid calling `shouldRevalidate` for fetchers that have not yet completed a data load diff --git a/package.json b/package.json index 4d1c08969e..05550524b8 100644 --- a/package.json +++ b/package.json @@ -109,7 +109,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "46.4 kB" + "none": "46.5 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13.8 kB" diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 1bf9627e5e..50d7955a05 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -10845,6 +10845,62 @@ describe("a router", () => { }); expect(t.router.state.fetchers.get(actionKey)).toBeUndefined(); }); + + it("does not call shouldRevalidate if fetcher has no data (called 2x rapidly)", async () => { + // This is specifically for a Remix use case where the initial fetcher.load + // call hasn't completed (and hasn't even loaded the route module yet), so + // there isn't even a shouldRevalidate implementation to access yet. If + // there's no data it should just interrupt the existing load and load again, + // it's not a "revalidation" + let spy = jest.fn(() => true); + let t = setup({ + routes: [ + { + id: "root", + path: "/", + children: [ + { + index: true, + }, + { + path: "page", + }, + ], + }, + { + id: "fetch", + path: "/fetch", + loader: true, + shouldRevalidate: spy, + }, + ], + }); + + let key = "key"; + let A = await t.fetch("/fetch", key, "root"); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + expect(A.loaders.fetch.signal.aborted).toBe(false); + + // This should trigger an automatic revalidation of the fetcher since it + // hasn't loaded yet + let B = await t.navigate("/page", undefined, ["fetch"]); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + expect(A.loaders.fetch.signal.aborted).toBe(true); + expect(B.loaders.fetch.signal.aborted).toBe(false); + + // No-op since the original call was aborted + await A.loaders.fetch.resolve("A"); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // Complete the navigation + await B.loaders.fetch.resolve("B"); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "B", + }); + expect(spy).not.toHaveBeenCalled(); + }); }); describe("fetcher ?index params", () => { diff --git a/packages/router/router.ts b/packages/router/router.ts index 34d45e5249..6de4ab07f5 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1481,6 +1481,7 @@ export function createRouter(init: RouterInit): Router { cancelledDeferredRoutes, cancelledFetcherLoads, fetchLoadMatches, + fetchRedirectIds, routesToUse, basename, pendingActionData, @@ -1539,6 +1540,9 @@ export function createRouter(init: RouterInit): Router { pendingNavigationLoadId = ++incrementingLoadId; revalidatingFetchers.forEach((rf) => { + if (fetchControllers.has(rf.key)) { + abortFetcher(rf.key); + } if (rf.controller) { // Fetchers use an independent AbortController so that aborting a fetcher // (via deleteFetcher) does not abort the triggering navigation that @@ -1806,6 +1810,7 @@ export function createRouter(init: RouterInit): Router { cancelledDeferredRoutes, cancelledFetcherLoads, fetchLoadMatches, + fetchRedirectIds, routesToUse, basename, { [match.route.id]: actionResult.data }, @@ -1825,6 +1830,9 @@ export function createRouter(init: RouterInit): Router { existingFetcher ? existingFetcher.data : undefined ); state.fetchers.set(staleKey, revalidatingFetcher); + if (fetchControllers.has(staleKey)) { + abortFetcher(staleKey); + } if (rf.controller) { fetchControllers.set(staleKey, rf.controller); } @@ -3276,6 +3284,7 @@ function getMatchesToLoad( cancelledDeferredRoutes: string[], cancelledFetcherLoads: string[], fetchLoadMatches: Map, + fetchRedirectIds: Set, routesToUse: AgnosticDataRouteObject[], basename: string | undefined, pendingActionData?: RouteData, @@ -3361,34 +3370,38 @@ function getMatchesToLoad( return; } + // Revalidating fetchers are decoupled from the route matches since they + // load from a static href. They only set `defaultShouldRevalidate` on + // explicit revalidation due to submission, useRevalidator, or X-Remix-Revalidate + // + // They automatically revalidate without even calling shouldRevalidate if: + // - They were cancelled + // - They're in the middle of their first load and therefore this is still + // an initial load and not a revalidation + // + // If neither of those is true, then they _always_ check shouldRevalidate + let fetcher = state.fetchers.get(key); + let isPerformingInitialLoad = + fetcher && + fetcher.state !== "idle" && + fetcher.data === undefined && + // If a fetcher.load redirected then it'll be "loading" without any data + // so ensure we're not processing the redirect from this fetcher + !fetchRedirectIds.has(key); let fetcherMatch = getTargetMatch(fetcherMatches, f.path); - - if (cancelledFetcherLoads.includes(key)) { - revalidatingFetchers.push({ - key, - routeId: f.routeId, - path: f.path, - matches: fetcherMatches, - match: fetcherMatch, - controller: new AbortController(), + let shouldRevalidate = + cancelledFetcherLoads.includes(key) || + isPerformingInitialLoad || + shouldRevalidateLoader(fetcherMatch, { + currentUrl, + currentParams: state.matches[state.matches.length - 1].params, + nextUrl, + nextParams: matches[matches.length - 1].params, + ...submission, + actionResult, + defaultShouldRevalidate: isRevalidationRequired, }); - return; - } - // Revalidating fetchers are decoupled from the route matches since they - // hit a static href, so they _always_ check shouldRevalidate and the - // default is strictly if a revalidation is explicitly required (action - // submissions, useRevalidator, X-Remix-Revalidate). - let shouldRevalidate = shouldRevalidateLoader(fetcherMatch, { - currentUrl, - currentParams: state.matches[state.matches.length - 1].params, - nextUrl, - nextParams: matches[matches.length - 1].params, - ...submission, - actionResult, - // Forced revalidation due to submission, useRevalidator, or X-Remix-Revalidate - defaultShouldRevalidate: isRevalidationRequired, - }); if (shouldRevalidate) { revalidatingFetchers.push({ key,