diff --git a/.changeset/initial-load-fetcher.md b/.changeset/initial-load-fetcher.md new file mode 100644 index 0000000000..c30f500787 --- /dev/null +++ b/.changeset/initial-load-fetcher.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Initial-load fetchers should not automatically revalidate on GET navigations diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 23d2e6b072..5b6910de1f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -10013,11 +10013,41 @@ describe("a router", () => { }); }); + describe(` + A) fetch GET /foo |--R + B) nav GET /bar |---O + `, () => { + it("ignores loader redirect navigation if preceded by a normal GET 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"); + + // The fetcher loader redirect should be ignored + await A.loaders.foo.redirect("/baz"); + expect(t.router.state.fetchers.get(key)?.state).toBe("idle"); + + 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 POST /foo |--R B) nav GET /bar |---O `, () => { - it("ignores submission redirect navigation if preceded by a normal navigation", async () => { + it("ignores submission redirect navigation if preceded by a normal GET navigation", async () => { let key = "key"; let t = initializeTmTest(); let A = await t.fetch("/foo", key, { @@ -10046,16 +10076,20 @@ describe("a router", () => { }); describe(` - A) fetch GET /foo |--R - B) nav GET /bar |---O + A) fetch GET /foo |--R |---O + B) nav POST /bar |--|--|---O `, () => { - it("ignores loader redirect navigation if preceded by a normal navigation", async () => { + it("ignores loader redirect navigation if preceded by a normal POST navigation", async () => { let key = "key"; let t = initializeTmTest(); - // Start a fetch load and interrupt with a navigation + // Start a fetch load and interrupt with a POST navigation let A = await t.fetch("/foo", key); - let B = await t.navigate("/bar", undefined, ["foo"]); + let B = await t.navigate( + "/bar", + { formMethod: "post", formData: createFormData({}) }, + ["foo"] + ); // The fetcher loader redirect should be ignored await A.loaders.foo.redirect("/baz"); @@ -10064,6 +10098,8 @@ describe("a router", () => { // 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.actions.bar.resolve("ACTION"); + await B.loaders.root.resolve("ROOT*"); await B.loaders.bar.resolve("BAR"); await B.loaders.foo.resolve("FOO"); @@ -10071,7 +10107,7 @@ describe("a router", () => { navigation: IDLE_NAVIGATION, location: { pathname: "/bar" }, loaderData: { - root: "ROOT", + root: "ROOT*", bar: "BAR", }, }); @@ -10079,13 +10115,18 @@ describe("a router", () => { expect(t.router.state.fetchers.get(key)?.data).toBe("FOO"); }); - it("processes second fetcher load redirect after interruption by normal navigation", async () => { + it("processes second fetcher load redirect after interruption by normal POST navigation", async () => { let key = "key"; let t = initializeTmTest(); - // Start a fetch load and interrupt with a navigation + // Start a fetch load and interrupt with a POST navigation let A = await t.fetch("/foo", key, "root"); - let B = await t.navigate("/bar", undefined, ["foo"]); + let B = await t.navigate( + "/bar", + { formMethod: "post", formData: createFormData({}) }, + ["foo"] + ); + expect(A.loaders.foo.signal.aborted).toBe(true); // The fetcher loader redirect should be ignored await A.loaders.foo.redirect("/baz"); @@ -10097,6 +10138,8 @@ describe("a router", () => { // The navigation should trigger the fetcher to revalidate since it's // not yet "completed". If it redirects again we should follow that + await B.actions.bar.resolve("ACTION"); + await B.loaders.root.resolve("ROOT*"); await B.loaders.bar.resolve("BAR"); let C = await B.loaders.foo.redirect( "/foo/bar", @@ -10114,20 +10157,26 @@ describe("a router", () => { expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); // The fetcher should not revalidate here since it triggered the redirect + await C.loaders.root.resolve("ROOT**"); await C.loaders.foobar.resolve("FOOBAR"); expect(t.router.state).toMatchObject({ navigation: IDLE_NAVIGATION, location: { pathname: "/foo/bar" }, loaderData: { - root: "ROOT", + 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 () => { + describe(` + A) fetch GET /foo |-----X + B) fetch GET /bar |---R + `, () => { + it("handles racing fetcher loader redirects", async () => { let keyA = "a"; let keyB = "b"; let t = initializeTmTest(); @@ -10136,11 +10185,8 @@ describe("a router", () => { 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", - ]); + // Return a redirect from the second fetcher.load + let C = await B.loaders.bar.redirect("/baz"); expect(t.router.state).toMatchObject({ navigation: { location: { pathname: "/baz" } }, location: { pathname: "/" }, @@ -10154,32 +10200,17 @@ describe("a router", () => { navigation: { location: { pathname: "/baz" } }, location: { pathname: "/" }, }); - expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading"); + expect(t.router.state.fetchers.get(keyA)?.state).toBe("idle"); expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading"); - // Resolve the navigation loader and the revalidating (first) fetcher - // loader which redirects again + // Resolve the navigation loader 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" }, + location: { pathname: "/baz" }, loaderData: { root: "ROOT", - foobar: "FOOBAR", + baz: "BAZ", }, }); expect(t.router.state.fetchers.get(keyA)?.state).toBe("idle"); @@ -11023,7 +11054,7 @@ describe("a router", () => { expect(t.router.state.fetchers.get(actionKey)).toBeUndefined(); }); - it("does not call shouldRevalidate if fetcher has no data (called 2x rapidly)", async () => { + it("does not call shouldRevalidate on POST navigation if fetcher has not yet loaded", async () => { // This is specifically for a Remix use case where the initial fetcher.load // call hasn't completed (and hasn't even loaded the route module yet), so // there isn't even a shouldRevalidate implementation to access yet. If @@ -11040,7 +11071,9 @@ describe("a router", () => { index: true, }, { + id: "page", path: "page", + action: true, }, ], }, @@ -11056,11 +11089,15 @@ describe("a router", () => { let key = "key"; let A = await t.fetch("/fetch", key, "root"); expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); - expect(A.loaders.fetch.signal.aborted).toBe(false); // This should trigger an automatic revalidation of the fetcher since it // hasn't loaded yet - let B = await t.navigate("/page", undefined, ["fetch"]); + let B = await t.navigate( + "/page", + { formMethod: "post", body: createFormData({}) }, + ["fetch"] + ); + await B.actions.page.resolve("ACTION"); expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); expect(A.loaders.fetch.signal.aborted).toBe(true); expect(B.loaders.fetch.signal.aborted).toBe(false); @@ -11078,6 +11115,54 @@ describe("a router", () => { }); expect(spy).not.toHaveBeenCalled(); }); + + it("does not trigger revalidation on GET navigation if fetcher has not yet loaded", async () => { + let spy = jest.fn(() => true); + let t = setup({ + routes: [ + { + id: "root", + path: "/", + children: [ + { + index: true, + }, + { + id: "page", + path: "page", + loader: true, + }, + ], + }, + { + id: "fetch", + path: "/fetch", + loader: true, + shouldRevalidate: spy, + }, + ], + }); + + let key = "key"; + let A = await t.fetch("/fetch", key, "root"); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + + let B = await t.navigate("/page"); + expect(t.router.state.fetchers.get(key)?.state).toBe("loading"); + expect(A.loaders.fetch.signal.aborted).toBe(false); + + await A.loaders.fetch.resolve("A"); + expect(t.router.state.fetchers.get(key)?.state).toBe("idle"); + + // Complete the navigation + await B.loaders.page.resolve("PAGE"); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.fetchers.get(key)).toMatchObject({ + state: "idle", + data: "A", + }); + expect(spy).not.toHaveBeenCalled(); + }); }); describe("fetcher ?index params", () => { diff --git a/packages/router/router.ts b/packages/router/router.ts index 79dd9959c7..adae0211e0 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -3398,7 +3398,9 @@ function getMatchesToLoad( let fetcherMatches = matchRoutes(routesToUse, f.path, basename); // If the fetcher path no longer matches, push it in with null matches so - // we can trigger a 404 in callLoadersAndMaybeResolveData + // we can trigger a 404 in callLoadersAndMaybeResolveData. Note this is + // currently only a use-case for Remix HMR where the route tree can change + // at runtime and remove a route previously loaded via a fetcher if (!fetcherMatches) { revalidatingFetchers.push({ key, @@ -3412,28 +3414,31 @@ function getMatchesToLoad( } // Revalidating fetchers are decoupled from the route matches since they - // load from a static href. They only set `defaultShouldRevalidate` on - // explicit revalidation due to submission, useRevalidator, or X-Remix-Revalidate - // - // They automatically revalidate without even calling shouldRevalidate if: - // - They were cancelled - // - They're in the middle of their first load and therefore this is still - // an initial load and not a revalidation - // - // If neither of those is true, then they _always_ check shouldRevalidate + // load from a static href. They revalidate based on explicit revalidation + // (submission, useRevalidator, or X-Remix-Revalidate) let fetcher = state.fetchers.get(key); - let isPerformingInitialLoad = + let fetcherMatch = getTargetMatch(fetcherMatches, f.path); + + let shouldRevalidate = false; + 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 + shouldRevalidate = true; + } else if ( fetcher && fetcher.state !== "idle" && - fetcher.data === undefined && - // If a fetcher.load redirected then it'll be "loading" without any data - // so ensure we're not processing the redirect from this fetcher - !fetchRedirectIds.has(key); - let fetcherMatch = getTargetMatch(fetcherMatches, f.path); - let shouldRevalidate = - cancelledFetcherLoads.includes(key) || - isPerformingInitialLoad || - shouldRevalidateLoader(fetcherMatch, { + fetcher.data === undefined + ) { + // If the fetcher hasn't ever completed loading yet, then this isn't a + // revalidation, it would just be a brand new load if an explicit + // revalidation is required + shouldRevalidate = isRevalidationRequired; + } else { + // Otherwise fall back on any user-defined shouldRevalidate, defaulting + // to explicit revalidations only + shouldRevalidate = shouldRevalidateLoader(fetcherMatch, { currentUrl, currentParams: state.matches[state.matches.length - 1].params, nextUrl, @@ -3442,6 +3447,7 @@ function getMatchesToLoad( actionResult, defaultShouldRevalidate: isRevalidationRequired, }); + } if (shouldRevalidate) { revalidatingFetchers.push({