From 26dce2398634b7342e062308f396ee2f96f017b4 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 6 Jul 2023 16:33:02 -0400 Subject: [PATCH] Properly handle fetcher redirects interrupted by normal navigations (#10674) --- .changeset/fetcher-redirect-interrupt.md | 5 + package.json | 2 +- packages/router/__tests__/router-test.ts | 179 ++++++++++++++++++++++- packages/router/router.ts | 72 +++++++-- 4 files changed, 240 insertions(+), 18 deletions(-) create mode 100644 .changeset/fetcher-redirect-interrupt.md diff --git a/.changeset/fetcher-redirect-interrupt.md b/.changeset/fetcher-redirect-interrupt.md new file mode 100644 index 0000000000..ce36bc296c --- /dev/null +++ b/.changeset/fetcher-redirect-interrupt.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Properly handle fetcher redirects interrupted by normal navigations diff --git a/package.json b/package.json index 26d65783e7..083bcd3098 100644 --- a/package.json +++ b/package.json @@ -109,7 +109,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "46.6 kB" + "none": "46.9 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 6feee2adc6..23d2e6b072 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -9726,7 +9726,10 @@ describe("a router", () => { expect(A.loaders.foo.signal.aborted).toBe(false); expect(t.router.state.navigation.state).toBe("idle"); expect(t.router.state.location.pathname).toBe("/foo"); - expect(t.router.state.loaderData.foo).toBe("B"); + expect(t.router.state.loaderData).toEqual({ + root: "B root", + foo: "B", + }); await A.loaders.root.resolve("A root"); await A.loaders.foo.resolve("A"); @@ -10009,6 +10012,180 @@ describe("a router", () => { expect(t.router.state.fetchers.get(key)?.data).toBeUndefined(); }); }); + + describe(` + A) fetch POST /foo |--R + B) nav GET /bar |---O + `, () => { + it("ignores submission redirect navigation if preceded by a normal navigation", async () => { + let key = "key"; + let t = initializeTmTest(); + let A = await t.fetch("/foo", key, { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + let B = await t.navigate("/bar"); + + // This redirect should be ignored + await A.actions.foo.redirect("/baz"); + expect(t.router.state.fetchers.get(key)?.state).toBe("idle"); + + await B.loaders.root.resolve("ROOT*"); + await B.loaders.bar.resolve("BAR"); + expect(t.router.state).toMatchObject({ + navigation: IDLE_NAVIGATION, + location: { pathname: "/bar" }, + loaderData: { + root: "ROOT*", + bar: "BAR", + }, + }); + expect(t.router.state.fetchers.get(key)?.state).toBe("idle"); + expect(t.router.state.fetchers.get(key)?.data).toBeUndefined(); + }); + }); + + describe(` + A) fetch GET /foo |--R + B) nav GET /bar |---O + `, () => { + it("ignores loader redirect navigation if preceded by a normal navigation", async () => { + let key = "key"; + let t = initializeTmTest(); + + // Start a fetch load and interrupt with a navigation + let A = await t.fetch("/foo", key); + let B = await t.navigate("/bar", undefined, ["foo"]); + + // The fetcher loader redirect should be ignored + await A.loaders.foo.redirect("/baz"); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // The navigation should trigger the fetcher to revalidate since it's + // not yet "completed". If it returns data this time that should be + // reflected + await B.loaders.bar.resolve("BAR"); + await B.loaders.foo.resolve("FOO"); + + expect(t.router.state).toMatchObject({ + navigation: IDLE_NAVIGATION, + location: { pathname: "/bar" }, + loaderData: { + root: "ROOT", + bar: "BAR", + }, + }); + expect(t.router.state.fetchers.get(key)?.state).toBe("idle"); + expect(t.router.state.fetchers.get(key)?.data).toBe("FOO"); + }); + + it("processes second fetcher load redirect after interruption by normal navigation", async () => { + let key = "key"; + let t = initializeTmTest(); + + // Start a fetch load and interrupt with a navigation + let A = await t.fetch("/foo", key, "root"); + let B = await t.navigate("/bar", undefined, ["foo"]); + + // The fetcher loader redirect should be ignored + await A.loaders.foo.redirect("/baz"); + expect(t.router.state).toMatchObject({ + navigation: { location: { pathname: "/bar" } }, + location: { pathname: "/" }, + }); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // The navigation should trigger the fetcher to revalidate since it's + // not yet "completed". If it redirects again we should follow that + await B.loaders.bar.resolve("BAR"); + let C = await B.loaders.foo.redirect( + "/foo/bar", + undefined, + undefined, + ["foo"] + ); + expect(t.router.state).toMatchObject({ + navigation: { location: { pathname: "/foo/bar" } }, + location: { pathname: "/" }, + loaderData: { + root: "ROOT", + }, + }); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + // The fetcher should not revalidate here since it triggered the redirect + await C.loaders.foobar.resolve("FOOBAR"); + expect(t.router.state).toMatchObject({ + navigation: IDLE_NAVIGATION, + location: { pathname: "/foo/bar" }, + loaderData: { + root: "ROOT", + foobar: "FOOBAR", + }, + }); + expect(t.router.state.fetchers.get(key)?.state).toBe("idle"); + expect(t.router.state.fetchers.get(key)?.data).toBe(undefined); + }); + + it("handle multiple fetcher loader redirects", async () => { + let keyA = "a"; + let keyB = "b"; + let t = initializeTmTest(); + + // Start 2 fetch loads + let A = await t.fetch("/foo", keyA, "root"); + let B = await t.fetch("/bar", keyB, "root"); + + // Return a redirect from the second fetcher.load (which will trigger + // a revalidation of the first fetcher) + let C = await B.loaders.bar.redirect("/baz", undefined, undefined, [ + "foo", + ]); + expect(t.router.state).toMatchObject({ + navigation: { location: { pathname: "/baz" } }, + location: { pathname: "/" }, + }); + expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading"); + expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading"); + + // The original fetch load redirect should be ignored + await A.loaders.foo.redirect("/foo/bar"); + expect(t.router.state).toMatchObject({ + navigation: { location: { pathname: "/baz" } }, + location: { pathname: "/" }, + }); + expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading"); + expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading"); + + // Resolve the navigation loader and the revalidating (first) fetcher + // loader which redirects again + await C.loaders.baz.resolve("BAZ"); + let D = await C.loaders.foo.redirect("/foo/bar"); + expect(t.router.state).toMatchObject({ + navigation: { location: { pathname: "/foo/bar" } }, + location: { pathname: "/" }, + loaderData: { + root: "ROOT", + }, + }); + expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading"); + expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading"); + + // Resolve the navigation loader, bringing everything back to idle at + // the final location + await D.loaders.foobar.resolve("FOOBAR"); + expect(t.router.state).toMatchObject({ + navigation: IDLE_NAVIGATION, + location: { pathname: "/foo/bar" }, + loaderData: { + root: "ROOT", + foobar: "FOOBAR", + }, + }); + expect(t.router.state.fetchers.get(keyA)?.state).toBe("idle"); + expect(t.router.state.fetchers.get(keyB)?.state).toBe("idle"); + }); + }); }); describe("fetcher revalidation", () => { diff --git a/packages/router/router.ts b/packages/router/router.ts index 785ec73b2a..79dd9959c7 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1591,7 +1591,15 @@ export function createRouter(init: RouterInit): Router { // If any loaders returned a redirect Response, start a new REPLACE navigation let redirect = findRedirect(results); if (redirect) { - await startRedirectNavigation(state, redirect, { replace }); + if (redirect.idx >= matchesToLoad.length) { + // If this redirect came from a fetcher make sure we mark it in + // fetchRedirectIds so it doesn't get revalidated on the next set of + // loader executions + let fetcherKey = + revalidatingFetchers[redirect.idx - matchesToLoad.length].key; + fetchRedirectIds.add(fetcherKey); + } + await startRedirectNavigation(state, redirect.result, { replace }); return { shortCircuited: true }; } @@ -1739,6 +1747,7 @@ export function createRouter(init: RouterInit): Router { ); fetchControllers.set(key, abortController); + let originatingLoadId = incrementingLoadId; let actionResult = await callLoaderOrAction( "action", fetchRequest, @@ -1760,15 +1769,26 @@ export function createRouter(init: RouterInit): Router { if (isRedirectResult(actionResult)) { fetchControllers.delete(key); - fetchRedirectIds.add(key); - let loadingFetcher = getLoadingFetcher(submission); - state.fetchers.set(key, loadingFetcher); - updateState({ fetchers: new Map(state.fetchers) }); - - return startRedirectNavigation(state, actionResult, { - submission, - isFetchActionRedirect: true, - }); + 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 + let doneFetcher = getDoneFetcher(undefined); + state.fetchers.set(key, doneFetcher); + updateState({ fetchers: new Map(state.fetchers) }); + return; + } else { + fetchRedirectIds.add(key); + let loadingFetcher = getLoadingFetcher(submission); + state.fetchers.set(key, loadingFetcher); + updateState({ fetchers: new Map(state.fetchers) }); + + return startRedirectNavigation(state, actionResult, { + submission, + isFetchActionRedirect: true, + }); + } } // Process any non-redirect errors thrown @@ -1875,7 +1895,15 @@ export function createRouter(init: RouterInit): Router { let redirect = findRedirect(results); if (redirect) { - return startRedirectNavigation(state, redirect); + if (redirect.idx >= matchesToLoad.length) { + // If this redirect came from a fetcher make sure we mark it in + // fetchRedirectIds so it doesn't get revalidated on the next set of + // loader executions + let fetcherKey = + revalidatingFetchers[redirect.idx - matchesToLoad.length].key; + fetchRedirectIds.add(fetcherKey); + } + return startRedirectNavigation(state, redirect.result); } // Process and commit output from loaders @@ -1962,6 +1990,7 @@ export function createRouter(init: RouterInit): Router { ); fetchControllers.set(key, abortController); + let originatingLoadId = incrementingLoadId; let result: DataResult = await callLoaderOrAction( "loader", fetchRequest, @@ -1994,9 +2023,18 @@ 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; + if (pendingNavigationLoadId > originatingLoadId) { + // A new navigation was kicked off after our loader started, so that + // should take precedence over this redirect navigation + let doneFetcher = getDoneFetcher(undefined); + state.fetchers.set(key, doneFetcher); + updateState({ fetchers: new Map(state.fetchers) }); + return; + } else { + fetchRedirectIds.add(key); + await startRedirectNavigation(state, result); + return; + } } // Process any non-redirect errors thrown @@ -4090,11 +4128,13 @@ function getInternalRouterError( } // Find any returned redirect errors, starting from the lowest match -function findRedirect(results: DataResult[]): RedirectResult | undefined { +function findRedirect( + results: DataResult[] +): { result: RedirectResult; idx: number } | undefined { for (let i = results.length - 1; i >= 0; i--) { let result = results[i]; if (isRedirectResult(result)) { - return result; + return { result, idx: i }; } } }