diff --git a/.changeset/hip-poets-provide.md b/.changeset/hip-poets-provide.md new file mode 100644 index 0000000000..9740048a14 --- /dev/null +++ b/.changeset/hip-poets-provide.md @@ -0,0 +1,11 @@ +--- +"@remix-run/router": patch +--- + +Fix internal cleanup of interrupted fetchers to avoid invalid revalidations on navigations + +- When a `fetcher.load` is interrupted by an `action` submission, we track it internally and force revalidation once the `action` completes +- We previously only cleared out this internal tracking info on a successful _navigation_ submission +- Therefore, if the `fetcher.load` was interrupted by a `fetcher.submit`, then we wouldn't remove it from this internal tracking info on successful load (incorrectly) +- And then on the next navigation it's presence in the internal tracking would automatically trigger execution of the `fetcher.load` again, ignoring any `shouldRevalidate` logic +- This fix cleans up the internal tracking so it applies to both navigation submission and fetcher submissions diff --git a/packages/router/__tests__/fetchers-test.ts b/packages/router/__tests__/fetchers-test.ts index 0860f2e3bf..7394fb3a93 100644 --- a/packages/router/__tests__/fetchers-test.ts +++ b/packages/router/__tests__/fetchers-test.ts @@ -2405,6 +2405,108 @@ describe("fetchers", () => { }); }); + it("cancels in-flight fetcher.loads on fetcher.action submission and forces reload (one time)", async () => { + let t = setup({ + routes: [ + { + id: "root", + path: "/", + children: [ + { + id: "index", + index: true, + }, + { + id: "page", + path: "page", + action: true, + }, + { + id: "action", + path: "action", + action: true, + }, + { + id: "fetch", + path: "fetch", + loader: true, + shouldRevalidate: () => false, + }, + ], + }, + ], + }); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + + // Kick off a fetch from the root route + let keyA = "a"; + let A = await t.fetch("/fetch", keyA, "root"); + expect(t.router.state.fetchers.get(keyA)).toMatchObject({ + state: "loading", + data: undefined, + }); + + // Interrupt with a fetcher submission which will trigger a forced + // revalidation, ignoring shouldRevalidate=false since the prior load + // was interrupted + let keyB = "b"; + let B = await t.fetch("/action", keyB, { + formMethod: "post", + formData: createFormData({}), + }); + t.shimHelper(B.loaders, "fetch", "loader", "fetch"); + expect(t.router.state.fetchers.get(keyB)).toMatchObject({ + state: "submitting", + data: undefined, + }); + + // Resolving the first load is a no-op since it would have been aborted + await A.loaders.fetch.resolve("A LOADER"); + expect(t.router.state.fetchers.get(keyA)).toMatchObject({ + state: "loading", + data: undefined, + }); + + // Resolve the action, kicking off revalidations which will force the + // fetcher "a" load to run again even through shouldRevalidate=false + await B.actions.action.resolve("B ACTION"); + expect(t.router.state.fetchers.get(keyB)).toMatchObject({ + state: "loading", + data: "B ACTION", + }); + expect(t.router.state.fetchers.get(keyA)).toMatchObject({ + state: "loading", + data: undefined, + }); + + // Resolve the second loader kicked off after the action + await B.loaders.fetch.resolve("B LOADER"); + expect(t.router.state.fetchers.get(keyA)).toMatchObject({ + state: "idle", + data: "B LOADER", + }); + + // submitting to another route + let C = await t.navigate("/page", { + formMethod: "post", + formData: createFormData({}), + }); + t.shimHelper(C.loaders, "navigation", "loader", "fetch"); + expect(t.router.state.navigation.location?.pathname).toBe("/page"); + + // should not trigger a fetcher reload due to shouldRevalidate=false + // This fixes a prior bug where we didn't remove the fetcher from + // cancelledFetcherLoaders upon the forced revalidation + await C.actions.page.resolve("PAGE ACTION"); + expect(t.router.state.location.pathname).toBe("/page"); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + expect(t.router.state.fetchers.get(keyA)).toMatchObject({ + state: "idle", + data: "B LOADER", + }); + expect(C.loaders.fetch.stub).not.toHaveBeenCalled(); + }); + it("does not cancel pending action navigation on deletion of revalidating fetcher", async () => { let t = setup({ routes: TASK_ROUTES, diff --git a/packages/router/router.ts b/packages/router/router.ts index 3c3ecf9051..05d19c4ccf 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -981,7 +981,7 @@ export function createRouter(init: RouterInit): Router { // Use this internal array to capture fetcher loads that were cancelled by an // action navigation and require revalidation - let cancelledFetcherLoads: string[] = []; + let cancelledFetcherLoads: Set = new Set(); // AbortControllers for any in-flight fetchers let fetchControllers = new Map(); @@ -1334,7 +1334,6 @@ export function createRouter(init: RouterInit): Router { isUninterruptedRevalidation = false; isRevalidationRequired = false; cancelledDeferredRoutes = []; - cancelledFetcherLoads = []; } // Trigger a navigation event, which can either be a numerical POP or a PUSH @@ -2867,7 +2866,7 @@ export function createRouter(init: RouterInit): Router { // Abort in-flight fetcher loads fetchLoadMatches.forEach((_, key) => { if (fetchControllers.has(key)) { - cancelledFetcherLoads.push(key); + cancelledFetcherLoads.add(key); abortFetcher(key); } }); @@ -2931,6 +2930,7 @@ export function createRouter(init: RouterInit): Router { fetchReloadIds.delete(key); fetchRedirectIds.delete(key); deletedFetchers.delete(key); + cancelledFetcherLoads.delete(key); state.fetchers.delete(key); } @@ -4324,7 +4324,7 @@ function getMatchesToLoad( skipActionErrorRevalidation: boolean, isRevalidationRequired: boolean, cancelledDeferredRoutes: string[], - cancelledFetcherLoads: string[], + cancelledFetcherLoads: Set, deletedFetchers: Set, fetchLoadMatches: Map, fetchRedirectIds: Set, @@ -4459,8 +4459,9 @@ function getMatchesToLoad( if (fetchRedirectIds.has(key)) { // Never trigger a revalidation of an actively redirecting fetcher shouldRevalidate = false; - } else if (cancelledFetcherLoads.includes(key)) { - // Always revalidate if the fetcher was cancelled + } else if (cancelledFetcherLoads.has(key)) { + // Always mark for revalidation if the fetcher was cancelled + cancelledFetcherLoads.delete(key); shouldRevalidate = true; } else if ( fetcher &&