diff --git a/.changeset/revalidating-fetcher-controller.md b/.changeset/revalidating-fetcher-controller.md new file mode 100644 index 0000000000..663eb9b3e3 --- /dev/null +++ b/.changeset/revalidating-fetcher-controller.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Decouple `AbortController` usage between revalidating fetchers and the thing that triggered them such that the unmount/deletion of a revalidating fetcher doesn't impact the ongoing triggering navigation/revalidation diff --git a/package.json b/package.json index c930322687..819d84e8e7 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "43.3 kB" + "none": "44 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB" diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 06d5cc4c1f..e4c4ef412f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -9836,7 +9836,7 @@ describe("a router", () => { state: "submitting", }); - // After acton resolves, both fetchers go into a loading state, with + // After action resolves, both fetchers go into a loading state, with // the load fetcher still reflecting it's stale data await C.actions.tasks.resolve("TASKS ACTION"); expect(t.router.state.fetchers.get(key)).toMatchObject({ @@ -10032,6 +10032,183 @@ describe("a router", () => { data: "C", }); }); + + it("does not cancel pending action navigation on deletion of revalidating fetcher", async () => { + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { loaderData: { root: "ROOT", index: "INDEX" } }, + }); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + + let key1 = "key1"; + let A = await t.fetch("/tasks/1", key1); + await A.loaders.tasksId.resolve("TASKS 1"); + + let C = await t.navigate("/tasks", { + formMethod: "post", + formData: createFormData({}), + }); + // Add a helper for the fetcher that will be revalidating + t.shimHelper(C.loaders, "navigation", "loader", "tasksId"); + + // Resolve the action + await C.actions.tasks.resolve("TASKS ACTION"); + + // Fetcher should go back into a loading state + expect(t.router.state.fetchers.get(key1)).toMatchObject({ + state: "loading", + data: "TASKS 1", + }); + + // Delete fetcher in the middle of the revalidation + t.router.deleteFetcher(key1); + expect(t.router.state.fetchers.get(key1)).toBeUndefined(); + + // Resolve navigation loaders + await C.loaders.root.resolve("ROOT*"); + await C.loaders.tasks.resolve("TASKS LOADER"); + + expect(t.router.state).toMatchObject({ + actionData: { + tasks: "TASKS ACTION", + }, + errors: null, + loaderData: { + tasks: "TASKS LOADER", + root: "ROOT*", + }, + }); + expect(t.router.state.fetchers.size).toBe(0); + }); + + it("does not cancel pending loader navigation on deletion of revalidating fetcher", async () => { + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { loaderData: { root: "ROOT", index: "INDEX" } }, + }); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + + let key1 = "key1"; + let A = await t.fetch("/tasks/1", key1); + await A.loaders.tasksId.resolve("TASKS 1"); + + // Loading navigation with query param to trigger revalidations + let C = await t.navigate("/tasks?key=value"); + + // Fetcher should go back into a loading state + expect(t.router.state.fetchers.get(key1)).toMatchObject({ + state: "loading", + data: "TASKS 1", + }); + + // Delete fetcher in the middle of the revalidation + t.router.deleteFetcher(key1); + expect(t.router.state.fetchers.get(key1)).toBeUndefined(); + + // Resolve navigation loaders + await C.loaders.root.resolve("ROOT*"); + await C.loaders.tasks.resolve("TASKS LOADER"); + + expect(t.router.state).toMatchObject({ + errors: null, + loaderData: { + tasks: "TASKS LOADER", + root: "ROOT*", + }, + }); + expect(t.router.state.fetchers.size).toBe(0); + }); + + it("does not cancel pending router.revalidate() on deletion of revalidating fetcher", async () => { + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { loaderData: { root: "ROOT", index: "INDEX" } }, + }); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + + let key1 = "key1"; + let A = await t.fetch("/tasks/1", key1); + await A.loaders.tasksId.resolve("TASKS 1"); + + // Trigger revalidations + let C = await t.revalidate(); + + // Fetcher should not go back into a loading state since it's a revalidation + expect(t.router.state.fetchers.get(key1)).toMatchObject({ + state: "idle", + data: "TASKS 1", + }); + + // Delete fetcher in the middle of the revalidation + t.router.deleteFetcher(key1); + expect(t.router.state.fetchers.get(key1)).toBeUndefined(); + + // Resolve navigation loaders + await C.loaders.root.resolve("ROOT*"); + await C.loaders.index.resolve("INDEX*"); + + expect(t.router.state).toMatchObject({ + errors: null, + loaderData: { + root: "ROOT*", + index: "INDEX*", + }, + }); + expect(t.router.state.fetchers.size).toBe(0); + }); + + it("does not cancel pending fetcher submission on deletion of revalidating fetcher", async () => { + let key = "key"; + let actionKey = "actionKey"; + let t = setup({ + routes: TASK_ROUTES, + initialEntries: ["/"], + hydrationData: { loaderData: { root: "ROOT", index: "INDEX" } }, + }); + + // Load a fetcher + let A = await t.fetch("/tasks/1", key); + await A.loaders.tasksId.resolve("TASKS ID"); + + // Submit a fetcher, leaves loaded fetcher untouched + let C = await t.fetch("/tasks", actionKey, { + formMethod: "post", + formData: createFormData({}), + }); + + // After action resolves, both fetchers go into a loading state, with + // the load fetcher still reflecting it's stale data + await C.actions.tasks.resolve("TASKS ACTION"); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "loading", + data: "TASKS ID", + }); + expect(t.router.state.fetchers.get(actionKey)).toMatchObject({ + state: "loading", + data: "TASKS ACTION", + }); + + // Delete fetcher in the middle of the revalidation + t.router.deleteFetcher(key); + expect(t.router.state.fetchers.get(key)).toBeUndefined(); + + // Resolve only active route loaders since fetcher was deleted + await C.loaders.root.resolve("ROOT*"); + await C.loaders.index.resolve("INDEX*"); + + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT*", + index: "INDEX*", + }); + expect(t.router.state.fetchers.get(key)).toBe(undefined); + expect(t.router.state.fetchers.get(actionKey)).toMatchObject({ + state: "idle", + data: "TASKS ACTION", + }); + }); }); describe("fetcher ?index params", () => { diff --git a/packages/router/router.ts b/packages/router/router.ts index 9e14bd0c11..9c518b3f34 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -593,6 +593,7 @@ interface RevalidatingFetcher extends FetchLoadMatch { key: string; match: AgnosticDataRouteMatch | null; matches: AgnosticDataRouteMatch[] | null; + controller: AbortController | null; } /** @@ -1495,9 +1496,24 @@ export function createRouter(init: RouterInit): Router { } pendingNavigationLoadId = ++incrementingLoadId; - revalidatingFetchers.forEach((rf) => - fetchControllers.set(rf.key, pendingNavigationController!) - ); + revalidatingFetchers.forEach((rf) => { + if (rf.controller) { + // Fetchers use an independent AbortController so that aborting a fetcher + // (via deleteFetcher) does not abort the triggering navigation that + // triggered the revalidation + fetchControllers.set(rf.key, rf.controller); + } + }); + + // Proxy navigation abort through to revalidation fetchers + let abortPendingFetchRevalidations = () => + revalidatingFetchers.forEach((f) => abortFetcher(f.key)); + if (pendingNavigationController) { + pendingNavigationController.signal.addEventListener( + "abort", + abortPendingFetchRevalidations + ); + } let { results, loaderResults, fetcherResults } = await callLoadersAndMaybeResolveData( @@ -1515,6 +1531,12 @@ export function createRouter(init: RouterInit): Router { // Clean up _after_ loaders have completed. Don't clean up if we short // circuited because fetchControllers would have been aborted and // reassigned to new controllers for the next navigation + if (pendingNavigationController) { + pendingNavigationController.signal.removeEventListener( + "abort", + abortPendingFetchRevalidations + ); + } revalidatingFetchers.forEach((rf) => fetchControllers.delete(rf.key)); // If any loaders returned a redirect Response, start a new REPLACE navigation @@ -1766,11 +1788,21 @@ export function createRouter(init: RouterInit): Router { " _hasFetcherDoneAnything ": true, }; state.fetchers.set(staleKey, revalidatingFetcher); - fetchControllers.set(staleKey, abortController); + if (rf.controller) { + fetchControllers.set(staleKey, rf.controller); + } }); updateState({ fetchers: new Map(state.fetchers) }); + let abortPendingFetchRevalidations = () => + revalidatingFetchers.forEach((rf) => abortFetcher(rf.key)); + + abortController.signal.addEventListener( + "abort", + abortPendingFetchRevalidations + ); + let { results, loaderResults, fetcherResults } = await callLoadersAndMaybeResolveData( state.matches, @@ -1784,6 +1816,11 @@ export function createRouter(init: RouterInit): Router { return; } + abortController.signal.removeEventListener( + "abort", + abortPendingFetchRevalidations + ); + fetchReloadIds.delete(key); fetchControllers.delete(key); revalidatingFetchers.forEach((r) => fetchControllers.delete(r.key)); @@ -2114,10 +2151,10 @@ export function createRouter(init: RouterInit): Router { ) ), ...fetchersToLoad.map((f) => { - if (f.matches && f.match) { + if (f.matches && f.match && f.controller) { return callLoaderOrAction( "loader", - createClientSideRequest(init.history, f.path, request.signal), + createClientSideRequest(init.history, f.path, f.controller.signal), f.match, f.matches, manifest, @@ -2141,7 +2178,7 @@ export function createRouter(init: RouterInit): Router { currentMatches, matchesToLoad, loaderResults, - request.signal, + loaderResults.map(() => request.signal), false, state.loaderData ), @@ -2149,7 +2186,7 @@ export function createRouter(init: RouterInit): Router { currentMatches, fetchersToLoad.map((f) => f.match), fetcherResults, - request.signal, + fetchersToLoad.map((f) => (f.controller ? f.controller.signal : null)), true ), ]); @@ -3126,7 +3163,14 @@ function getMatchesToLoad( // If the fetcher path no longer matches, push it in with null matches so // we can trigger a 404 in callLoadersAndMaybeResolveData if (!fetcherMatches) { - revalidatingFetchers.push({ key, ...f, matches: null, match: null }); + revalidatingFetchers.push({ + key, + routeId: f.routeId, + path: f.path, + matches: null, + match: null, + controller: null, + }); return; } @@ -3135,9 +3179,11 @@ function getMatchesToLoad( if (cancelledFetcherLoads.includes(key)) { revalidatingFetchers.push({ key, + routeId: f.routeId, + path: f.path, matches: fetcherMatches, match: fetcherMatch, - ...f, + controller: new AbortController(), }); return; } @@ -3158,9 +3204,11 @@ function getMatchesToLoad( if (shouldRevalidate) { revalidatingFetchers.push({ key, + routeId: f.routeId, + path: f.path, matches: fetcherMatches, match: fetcherMatch, - ...f, + controller: new AbortController(), }); } }); @@ -3659,7 +3707,7 @@ function processLoaderData( // Process results from our revalidating fetchers for (let index = 0; index < revalidatingFetchers.length; index++) { - let { key, match } = revalidatingFetchers[index]; + let { key, match, controller } = revalidatingFetchers[index]; invariant( fetcherResults !== undefined && fetcherResults[index] !== undefined, "Did not find corresponding fetcher result" @@ -3667,7 +3715,10 @@ function processLoaderData( let result = fetcherResults[index]; // Process fetcher non-redirect errors - if (isErrorResult(result)) { + if (controller && controller.signal.aborted) { + // Nothing to do for aborted fetchers + continue; + } else if (isErrorResult(result)) { let boundaryMatch = findNearestBoundary(state.matches, match?.route.id); if (!(errors && errors[boundaryMatch.route.id])) { errors = { @@ -3910,7 +3961,7 @@ async function resolveDeferredResults( currentMatches: AgnosticDataRouteMatch[], matchesToLoad: (AgnosticDataRouteMatch | null)[], results: DataResult[], - signal: AbortSignal, + signals: (AbortSignal | null)[], isFetcher: boolean, currentLoaderData?: RouteData ) { @@ -3936,6 +3987,11 @@ async function resolveDeferredResults( // Note: we do not have to touch activeDeferreds here since we race them // against the signal in resolveDeferredData and they'll get aborted // there if needed + let signal = signals[index]; + invariant( + signal, + "Expected an AbortSignal for revalidating fetcher deferred result" + ); await resolveDeferredData(result, signal, isFetcher).then((result) => { if (result) { results[index] = result || results[index];