From 54ec39ee35cfde99ee7218e6aa2bd1b7d8afc787 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 7 Dec 2023 16:34:08 -0500 Subject: [PATCH] Allow persisted fetchers unmounted during submit to revalidate (#11102) * Allow persisted fetchers unmounted during submit to revalidate * Bump bundle --- .changeset/silver-teachers-doubt.md | 5 + .../__tests__/data-browser-router-test.tsx | 4 +- packages/router/__tests__/fetchers-test.ts | 133 +++++++++++++++++- .../__tests__/utils/data-router-setup.ts | 2 +- packages/router/router.ts | 54 ++++--- 5 files changed, 171 insertions(+), 27 deletions(-) create mode 100644 .changeset/silver-teachers-doubt.md diff --git a/.changeset/silver-teachers-doubt.md b/.changeset/silver-teachers-doubt.md new file mode 100644 index 0000000000..16c714113c --- /dev/null +++ b/.changeset/silver-teachers-doubt.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix bug preventing revalidation from occuring for persisted fetchers unmounted during the `submitting` phase diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index 16b195c9ef..06c7f4e5ec 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -5712,10 +5712,10 @@ function testDomRouter( await waitFor(() => screen.getByText("Page (1)")); expect(getHtml(container)).toMatch("Num fetchers: 1"); - // Resolve after the navigation and revalidation + // Resolve action after the navigation and trigger revalidation dfd.resolve("FETCH"); await waitFor(() => screen.getByText("Num fetchers: 0")); - expect(getHtml(container)).toMatch("Page (1)"); + expect(getHtml(container)).toMatch("Page (2)"); }); it("submitting fetchers w/revalidations are cleaned up on completion (remounted)", async () => { diff --git a/packages/router/__tests__/fetchers-test.ts b/packages/router/__tests__/fetchers-test.ts index 4073a14df6..07ddfc356c 100644 --- a/packages/router/__tests__/fetchers-test.ts +++ b/packages/router/__tests__/fetchers-test.ts @@ -1,5 +1,5 @@ /* eslint-disable jest/valid-title */ -import type { HydrationState } from "../index"; +import type { FutureConfig, HydrationState } from "../index"; import { createMemoryHistory, createRouter, @@ -21,6 +21,7 @@ import { createFormData, tick } from "./utils/utils"; function initializeTest(init?: { url?: string; hydrationData?: HydrationState; + future?: Partial; }) { return setup({ routes: [ @@ -66,6 +67,7 @@ function initializeTest(init?: { hydrationData: init?.hydrationData || { loaderData: { root: "ROOT", index: "INDEX" }, }, + future: init?.future, ...(init?.url ? { initialEntries: [init.url] } : {}), }); } @@ -82,6 +84,7 @@ describe("fetchers", () => { // @ts-ignore console.warn.mockReset(); }); + describe("fetcher states", () => { it("unabstracted loader fetch", async () => { let dfd = createDeferred(); @@ -338,6 +341,134 @@ describe("fetchers", () => { }); }); + describe("fetcher removal (w/v7_fetcherPersist)", () => { + it("loading fetchers persist until completion", async () => { + let t = initializeTest({ future: { v7_fetcherPersist: true } }); + + let key = "key"; + t.router.getFetcher(key); // mount + expect(t.router.state.fetchers.size).toBe(0); + + let A = await t.fetch("/foo", key); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + t.router.deleteFetcher(key); // unmount + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // Cleaned up on completion + await A.loaders.foo.resolve("FOO"); + expect(t.router.state.fetchers.size).toBe(0); + }); + + it("submitting fetchers persist until completion when removed during submitting phase", async () => { + let t = initializeTest({ future: { v7_fetcherPersist: true } }); + + let key = "key"; + expect(t.router.state.fetchers.size).toBe(0); + + t.router.getFetcher(key); // mount + let A = await t.fetch("/foo", key, { + formMethod: "post", + formData: createFormData({}), + }); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("submitting"); + + t.router.deleteFetcher(key); // unmount + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("submitting"); + + await A.actions.foo.resolve("FOO"); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // Cleaned up on completion + await A.loaders.root.resolve("ROOT*"); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + await A.loaders.index.resolve("INDEX*"); + expect(t.router.state.fetchers.size).toBe(0); + }); + + it("submitting fetchers persist until completion when removed during loading phase", async () => { + let t = initializeTest({ future: { v7_fetcherPersist: true } }); + + let key = "key"; + t.router.getFetcher(key); // mount + expect(t.router.state.fetchers.size).toBe(0); + + let A = await t.fetch("/foo", key, { + formMethod: "post", + formData: createFormData({}), + }); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("submitting"); + + await A.actions.foo.resolve("FOO"); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + t.router.deleteFetcher(key); // unmount + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // Cleaned up on completion + await A.loaders.root.resolve("ROOT*"); + expect(t.router.state.fetchers.size).toBe(1); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + await A.loaders.index.resolve("INDEX*"); + expect(t.router.state.fetchers.size).toBe(0); + }); + + it("unmounted fetcher.load errors/redirects should not be processed", async () => { + let t = initializeTest({ future: { v7_fetcherPersist: true } }); + + t.router.getFetcher("a"); // mount + let A = await t.fetch("/foo", "a"); + t.router.deleteFetcher("a"); //unmount + await A.loaders.foo.reject("ERROR"); + expect(t.router.state.fetchers.size).toBe(0); + expect(t.router.state.errors).toBe(null); + + t.router.getFetcher("b"); // mount + let B = await t.fetch("/bar", "b"); + t.router.deleteFetcher("b"); //unmount + await B.loaders.bar.redirect("/baz"); + expect(t.router.state.fetchers.size).toBe(0); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + expect(t.router.state.location.pathname).toBe("/"); + }); + + it("unmounted fetcher.submit errors/redirects should not be processed", async () => { + let t = initializeTest({ future: { v7_fetcherPersist: true } }); + + t.router.getFetcher("a"); // mount + let A = await t.fetch("/foo", "a", { + formMethod: "post", + formData: createFormData({}), + }); + t.router.deleteFetcher("a"); //unmount + await A.actions.foo.reject("ERROR"); + expect(t.router.state.fetchers.size).toBe(0); + expect(t.router.state.errors).toBe(null); + + t.router.getFetcher("b"); // mount + let B = await t.fetch("/bar", "b", { + formMethod: "post", + formData: createFormData({}), + }); + t.router.deleteFetcher("b"); //unmount + await B.actions.bar.redirect("/baz"); + expect(t.router.state.fetchers.size).toBe(0); + expect(t.router.state.navigation).toBe(IDLE_NAVIGATION); + expect(t.router.state.location.pathname).toBe("/"); + }); + }); + describe("fetcher error states (4xx Response)", () => { it("loader fetch", async () => { let t = initializeTest(); diff --git a/packages/router/__tests__/utils/data-router-setup.ts b/packages/router/__tests__/utils/data-router-setup.ts index 94bcee48fa..40d73cb8ff 100644 --- a/packages/router/__tests__/utils/data-router-setup.ts +++ b/packages/router/__tests__/utils/data-router-setup.ts @@ -142,7 +142,7 @@ type SetupOpts = { initialEntries?: InitialEntry[]; initialIndex?: number; hydrationData?: HydrationState; - future?: FutureConfig; + future?: Partial; }; // We use a slightly modified version of createDeferred here that includes the diff --git a/packages/router/router.ts b/packages/router/router.ts index 5992912a11..a00f54e627 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1981,33 +1981,39 @@ export function createRouter(init: RouterInit): Router { return; } - if (deletedFetchers.has(key)) { - updateFetcherState(key, getDoneFetcher(undefined)); - return; - } - - if (isRedirectResult(actionResult)) { - fetchControllers.delete(key); - if (pendingNavigationLoadId > originatingLoadId) { - // A new navigation was kicked off after our action started, so that - // should take precedence over this redirect navigation. We already - // set isRevalidationRequired so all loaders for the new route should - // fire unless opted out via shouldRevalidate + // When using v7_fetcherPersist, we don't want errors bubbling up to the UI + // or redirects processed for unmounted fetchers so we just revert them to + // idle + if (future.v7_fetcherPersist && deletedFetchers.has(key)) { + if (isRedirectResult(actionResult) || isErrorResult(actionResult)) { updateFetcherState(key, getDoneFetcher(undefined)); return; - } else { - fetchRedirectIds.add(key); - updateFetcherState(key, getLoadingFetcher(submission)); - return startRedirectNavigation(state, actionResult, { - fetcherSubmission: submission, - }); } - } + // Let SuccessResult's fall through for revalidation + } else { + if (isRedirectResult(actionResult)) { + fetchControllers.delete(key); + if (pendingNavigationLoadId > originatingLoadId) { + // A new navigation was kicked off after our action started, so that + // should take precedence over this redirect navigation. We already + // set isRevalidationRequired so all loaders for the new route should + // fire unless opted out via shouldRevalidate + updateFetcherState(key, getDoneFetcher(undefined)); + return; + } else { + fetchRedirectIds.add(key); + updateFetcherState(key, getLoadingFetcher(submission)); + return startRedirectNavigation(state, actionResult, { + fetcherSubmission: submission, + }); + } + } - // Process any non-redirect errors thrown - if (isErrorResult(actionResult)) { - setFetcherError(key, routeId, actionResult.error); - return; + // Process any non-redirect errors thrown + if (isErrorResult(actionResult)) { + setFetcherError(key, routeId, actionResult.error); + return; + } } if (isDeferredResult(actionResult)) { @@ -2237,6 +2243,8 @@ export function createRouter(init: RouterInit): Router { return; } + // We don't want errors bubbling up or redirects followed for unmounted + // fetchers, so short circuit here if it was removed from the UI if (deletedFetchers.has(key)) { updateFetcherState(key, getDoneFetcher(undefined)); return;