diff --git a/.changeset/fix-fetcher-revalidation.md b/.changeset/fix-fetcher-revalidation.md new file mode 100644 index 0000000000..c150a2b12b --- /dev/null +++ b/.changeset/fix-fetcher-revalidation.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fixed a bug where fetchers were incorrectly attempting to revalidate on search params changes or routing to the same URL (using the same logic for route `loader` revalidations). However, since fetchers have a static href, they should only revalidate on `action` submissions or `router.revalidate` calls. diff --git a/package.json b/package.json index 69ce64c5a4..1310d4da63 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "44.1 kB" + "none": "44.2 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13.1 kB" diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index e4c4ef412f..d5d3723156 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -6249,17 +6249,10 @@ describe("a router", () => { let fetch = await t.fetch("/parent", "key"); - let B = await fetch.loaders.parent.redirectReturn( - "..", - undefined, - undefined, - ["parent"] - ); + await fetch.loaders.parent.redirectReturn("..", undefined, undefined, [ + "parent", + ]); - // We called fetcher.load('/parent') from the root route, so when we - // redirect back to the root it triggers a revalidation of the - // fetcher.load('/parent') - await B.loaders.parent.resolve("Revalidated"); // No root loader so redirect lands immediately expect(t.router.state).toMatchObject({ location: { @@ -6271,7 +6264,7 @@ describe("a router", () => { }); expect(t.router.state.fetchers.get("key")).toMatchObject({ state: "idle", - data: "Revalidated", + data: undefined, }); }); @@ -9578,7 +9571,7 @@ describe("a router", () => { }); }); - it("revalidates fetchers on searchParams changes", async () => { + it("does not revalidate fetchers on searchParams changes", async () => { let key = "key"; let t = setup({ routes: TASK_ROUTES, @@ -9601,15 +9594,15 @@ describe("a router", () => { let B = await t.navigate("/tasks/1?key=value", undefined, ["index"]); await B.loaders.root.resolve("ROOT 2"); await B.loaders.tasksId.resolve("TASK 2"); - await B.loaders.index.resolve("FETCH 2"); expect(t.router.state.loaderData).toMatchObject({ root: "ROOT 2", tasksId: "TASK 2", }); expect(t.router.state.fetchers.get(key)).toMatchObject({ state: "idle", - data: "FETCH 2", + data: "FETCH 1", }); + expect(B.loaders.index.stub).not.toHaveBeenCalled(); }); it("revalidates fetchers on links to the current location", async () => { @@ -9635,15 +9628,15 @@ describe("a router", () => { let B = await t.navigate("/tasks/1", undefined, ["index"]); await B.loaders.root.resolve("ROOT 2"); await B.loaders.tasksId.resolve("TASK 2"); - await B.loaders.index.resolve("FETCH 2"); expect(t.router.state.loaderData).toMatchObject({ root: "ROOT 2", tasksId: "TASK 2", }); expect(t.router.state.fetchers.get(key)).toMatchObject({ state: "idle", - data: "FETCH 2", + data: "FETCH 1", }); + expect(B.loaders.index.stub).not.toHaveBeenCalled(); }); it("does not revalidate idle fetchers when a loader navigation is performed", async () => { @@ -10094,8 +10087,12 @@ describe("a router", () => { 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"); + // Submission navigation to trigger revalidations + let C = await t.navigate("/tasks", { + formMethod: "post", + formData: createFormData({}), + }); + await C.actions.tasks.resolve("TASKS ACTION"); // Fetcher should go back into a loading state expect(t.router.state.fetchers.get(key1)).toMatchObject({ @@ -10107,12 +10104,16 @@ describe("a router", () => { t.router.deleteFetcher(key1); expect(t.router.state.fetchers.get(key1)).toBeUndefined(); - // Resolve navigation loaders + // Resolve navigation action/loaders await C.loaders.root.resolve("ROOT*"); await C.loaders.tasks.resolve("TASKS LOADER"); expect(t.router.state).toMatchObject({ errors: null, + navigation: IDLE_NAVIGATION, + actionData: { + tasks: "TASKS ACTION", + }, loaderData: { tasks: "TASKS LOADER", root: "ROOT*", diff --git a/packages/router/router.ts b/packages/router/router.ts index 3427af0149..4b8171fb51 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -814,7 +814,7 @@ export function createRouter(init: RouterInit): Router { // Fetchers that triggered data reloads as a result of their actions let fetchReloadIds = new Map(); - // Fetchers that triggered redirect navigations from their actions + // Fetchers that triggered redirect navigations let fetchRedirectIds = new Set(); // Most recent href/match for fetcher.load calls for fetchers @@ -1470,12 +1470,14 @@ export function createRouter(init: RouterInit): Router { // Short circuit if we have no loaders to run if (matchesToLoad.length === 0 && revalidatingFetchers.length === 0) { + let updatedFetchers = markFetchRedirectsDone(); completeNavigation(location, { matches, loaderData: {}, // Commit pending error if we're short circuiting errors: pendingError || null, ...(pendingActionData ? { actionData: pendingActionData } : {}), + ...(updatedFetchers ? { fetchers: new Map(state.fetchers) } : {}), }); return { shortCircuited: true }; } @@ -1587,15 +1589,15 @@ export function createRouter(init: RouterInit): Router { }); }); - markFetchRedirectsDone(); + let updatedFetchers = markFetchRedirectsDone(); let didAbortFetchLoads = abortStaleFetchLoads(pendingNavigationLoadId); + let shouldUpdateFetchers = + updatedFetchers || didAbortFetchLoads || revalidatingFetchers.length > 0; return { loaderData, errors, - ...(didAbortFetchLoads || revalidatingFetchers.length > 0 - ? { fetchers: new Map(state.fetchers) } - : {}), + ...(shouldUpdateFetchers ? { fetchers: new Map(state.fetchers) } : {}), }; } @@ -1959,7 +1961,7 @@ export function createRouter(init: RouterInit): Router { result; } - // We can delete this so long as we weren't aborted by ou our own fetcher + // We can delete this so long as we weren't aborted by our our own fetcher // re-load which would have put _new_ controller is in fetchControllers if (fetchControllers.get(key) === abortController) { fetchControllers.delete(key); @@ -1971,6 +1973,7 @@ export function createRouter(init: RouterInit): Router { // If the loader threw a redirect Response, start a new REPLACE navigation if (isRedirectResult(result)) { + fetchRedirectIds.add(key); await startRedirectNavigation(state, result); return; } @@ -2270,17 +2273,20 @@ export function createRouter(init: RouterInit): Router { } } - function markFetchRedirectsDone(): void { + function markFetchRedirectsDone(): boolean { let doneKeys = []; + let updatedFetchers = false; for (let key of fetchRedirectIds) { let fetcher = state.fetchers.get(key); invariant(fetcher, `Expected fetcher: ${key}`); if (fetcher.state === "loading") { fetchRedirectIds.delete(key); doneKeys.push(key); + updatedFetchers = true; } } markFetchersDone(doneKeys); + return updatedFetchers; } function abortStaleFetchLoads(landedId: number): boolean { @@ -3133,14 +3139,6 @@ function getMatchesToLoad( let currentUrl = history.createURL(state.location); let nextUrl = history.createURL(location); - let defaultShouldRevalidate = - // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate - isRevalidationRequired || - // Clicked the same link, resubmitted a GET form - currentUrl.toString() === nextUrl.toString() || - // Search params affect all loaders - currentUrl.search !== nextUrl.search; - // Pick navigation matches that are net-new or qualify for revalidation let boundaryId = pendingError ? Object.keys(pendingError)[0] : undefined; let boundaryMatches = getLoaderMatchesUntilBoundary(matches, boundaryId); @@ -3177,7 +3175,12 @@ function getMatchesToLoad( ...submission, actionResult, defaultShouldRevalidate: - defaultShouldRevalidate || + // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate + isRevalidationRequired || + // Clicked the same link, resubmitted a GET form + currentUrl.toString() === nextUrl.toString() || + // Search params affect all loaders + currentUrl.search !== nextUrl.search || isNewRouteInstance(currentRouteMatch, nextRouteMatch), }); }); @@ -3231,7 +3234,8 @@ function getMatchesToLoad( nextParams: matches[matches.length - 1].params, ...submission, actionResult, - defaultShouldRevalidate, + // Forced revalidation due to submission, useRevalidate, or X-Remix-Revalidate + defaultShouldRevalidate: isRevalidationRequired, }); if (shouldRevalidate) { revalidatingFetchers.push({