Skip to content

Commit 058be38

Browse files
brophdawg11brophdawg11
authored
Initial-load fetchers should not automatically revalidate on GET navigations (#10688)
1 parent 0586306 commit 058be38

File tree

3 files changed

+155
-59
lines changed

3 files changed

+155
-59
lines changed

‎.changeset/initial-load-fetcher.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@remix-run/router": patch
3+
---
4+
5+
Initial-load fetchers should not automatically revalidate on GET navigations

‎packages/router/__tests__/router-test.ts

Lines changed: 124 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -10013,11 +10013,41 @@ describe("a router", () => {
1001310013
});
1001410014
});
1001510015

10016+
describe(`
10017+
A) fetch GET /foo |--R
10018+
B) nav GET /bar |---O
10019+
`, () => {
10020+
it("ignores loader redirect navigation if preceded by a normal GET navigation", async () => {
10021+
let key = "key";
10022+
let t = initializeTmTest();
10023+
10024+
// Start a fetch load and interrupt with a navigation
10025+
let A = await t.fetch("/foo", key);
10026+
let B = await t.navigate("/bar");
10027+
10028+
// The fetcher loader redirect should be ignored
10029+
await A.loaders.foo.redirect("/baz");
10030+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
10031+
10032+
await B.loaders.bar.resolve("BAR");
10033+
expect(t.router.state).toMatchObject({
10034+
navigation: IDLE_NAVIGATION,
10035+
location: { pathname: "/bar" },
10036+
loaderData: {
10037+
root: "ROOT",
10038+
bar: "BAR",
10039+
},
10040+
});
10041+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
10042+
expect(t.router.state.fetchers.get(key)?.data).toBeUndefined();
10043+
});
10044+
});
10045+
1001610046
describe(`
1001710047
A) fetch POST /foo |--R
1001810048
B) nav GET /bar |---O
1001910049
`, () => {
10020-
it("ignores submission redirect navigation if preceded by a normal navigation", async () => {
10050+
it("ignores submission redirect navigation if preceded by a normal GET navigation", async () => {
1002110051
let key = "key";
1002210052
let t = initializeTmTest();
1002310053
let A = await t.fetch("/foo", key, {
@@ -10046,16 +10076,20 @@ describe("a router", () => {
1004610076
});
1004710077

1004810078
describe(`
10049-
A) fetch GET /foo |--R
10050-
B) nav GET /bar |---O
10079+
A) fetch GET /foo |--R |---O
10080+
B) nav POST /bar |--|--|---O
1005110081
`, () => {
10052-
it("ignores loader redirect navigation if preceded by a normal navigation", async () => {
10082+
it("ignores loader redirect navigation if preceded by a normal POST navigation", async () => {
1005310083
let key = "key";
1005410084
let t = initializeTmTest();
1005510085

10056-
// Start a fetch load and interrupt with a navigation
10086+
// Start a fetch load and interrupt with a POST navigation
1005710087
let A = await t.fetch("/foo", key);
10058-
let B = await t.navigate("/bar", undefined, ["foo"]);
10088+
let B = await t.navigate(
10089+
"/bar",
10090+
{ formMethod: "post", formData: createFormData({}) },
10091+
["foo"]
10092+
);
1005910093

1006010094
// The fetcher loader redirect should be ignored
1006110095
await A.loaders.foo.redirect("/baz");
@@ -10064,28 +10098,35 @@ describe("a router", () => {
1006410098
// The navigation should trigger the fetcher to revalidate since it's
1006510099
// not yet "completed". If it returns data this time that should be
1006610100
// reflected
10101+
await B.actions.bar.resolve("ACTION");
10102+
await B.loaders.root.resolve("ROOT*");
1006710103
await B.loaders.bar.resolve("BAR");
1006810104
await B.loaders.foo.resolve("FOO");
1006910105

1007010106
expect(t.router.state).toMatchObject({
1007110107
navigation: IDLE_NAVIGATION,
1007210108
location: { pathname: "/bar" },
1007310109
loaderData: {
10074-
root: "ROOT",
10110+
root: "ROOT*",
1007510111
bar: "BAR",
1007610112
},
1007710113
});
1007810114
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
1007910115
expect(t.router.state.fetchers.get(key)?.data).toBe("FOO");
1008010116
});
1008110117

10082-
it("processes second fetcher load redirect after interruption by normal navigation", async () => {
10118+
it("processes second fetcher load redirect after interruption by normal POST navigation", async () => {
1008310119
let key = "key";
1008410120
let t = initializeTmTest();
1008510121

10086-
// Start a fetch load and interrupt with a navigation
10122+
// Start a fetch load and interrupt with a POST navigation
1008710123
let A = await t.fetch("/foo", key, "root");
10088-
let B = await t.navigate("/bar", undefined, ["foo"]);
10124+
let B = await t.navigate(
10125+
"/bar",
10126+
{ formMethod: "post", formData: createFormData({}) },
10127+
["foo"]
10128+
);
10129+
expect(A.loaders.foo.signal.aborted).toBe(true);
1008910130

1009010131
// The fetcher loader redirect should be ignored
1009110132
await A.loaders.foo.redirect("/baz");
@@ -10097,6 +10138,8 @@ describe("a router", () => {
1009710138

1009810139
// The navigation should trigger the fetcher to revalidate since it's
1009910140
// not yet "completed". If it redirects again we should follow that
10141+
await B.actions.bar.resolve("ACTION");
10142+
await B.loaders.root.resolve("ROOT*");
1010010143
await B.loaders.bar.resolve("BAR");
1010110144
let C = await B.loaders.foo.redirect(
1010210145
"/foo/bar",
@@ -10114,20 +10157,26 @@ describe("a router", () => {
1011410157
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
1011510158

1011610159
// The fetcher should not revalidate here since it triggered the redirect
10160+
await C.loaders.root.resolve("ROOT**");
1011710161
await C.loaders.foobar.resolve("FOOBAR");
1011810162
expect(t.router.state).toMatchObject({
1011910163
navigation: IDLE_NAVIGATION,
1012010164
location: { pathname: "/foo/bar" },
1012110165
loaderData: {
10122-
root: "ROOT",
10166+
root: "ROOT**",
1012310167
foobar: "FOOBAR",
1012410168
},
1012510169
});
1012610170
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
1012710171
expect(t.router.state.fetchers.get(key)?.data).toBe(undefined);
1012810172
});
10173+
});
1012910174

10130-
it("handle multiple fetcher loader redirects", async () => {
10175+
describe(`
10176+
A) fetch GET /foo |-----X
10177+
B) fetch GET /bar |---R
10178+
`, () => {
10179+
it("handles racing fetcher loader redirects", async () => {
1013110180
let keyA = "a";
1013210181
let keyB = "b";
1013310182
let t = initializeTmTest();
@@ -10136,11 +10185,8 @@ describe("a router", () => {
1013610185
let A = await t.fetch("/foo", keyA, "root");
1013710186
let B = await t.fetch("/bar", keyB, "root");
1013810187

10139-
// Return a redirect from the second fetcher.load (which will trigger
10140-
// a revalidation of the first fetcher)
10141-
let C = await B.loaders.bar.redirect("/baz", undefined, undefined, [
10142-
"foo",
10143-
]);
10188+
// Return a redirect from the second fetcher.load
10189+
let C = await B.loaders.bar.redirect("/baz");
1014410190
expect(t.router.state).toMatchObject({
1014510191
navigation: { location: { pathname: "/baz" } },
1014610192
location: { pathname: "/" },
@@ -10154,32 +10200,17 @@ describe("a router", () => {
1015410200
navigation: { location: { pathname: "/baz" } },
1015510201
location: { pathname: "/" },
1015610202
});
10157-
expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading");
10203+
expect(t.router.state.fetchers.get(keyA)?.state).toBe("idle");
1015810204
expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading");
1015910205

10160-
// Resolve the navigation loader and the revalidating (first) fetcher
10161-
// loader which redirects again
10206+
// Resolve the navigation loader
1016210207
await C.loaders.baz.resolve("BAZ");
10163-
let D = await C.loaders.foo.redirect("/foo/bar");
10164-
expect(t.router.state).toMatchObject({
10165-
navigation: { location: { pathname: "/foo/bar" } },
10166-
location: { pathname: "/" },
10167-
loaderData: {
10168-
root: "ROOT",
10169-
},
10170-
});
10171-
expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading");
10172-
expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading");
10173-
10174-
// Resolve the navigation loader, bringing everything back to idle at
10175-
// the final location
10176-
await D.loaders.foobar.resolve("FOOBAR");
1017710208
expect(t.router.state).toMatchObject({
1017810209
navigation: IDLE_NAVIGATION,
10179-
location: { pathname: "/foo/bar" },
10210+
location: { pathname: "/baz" },
1018010211
loaderData: {
1018110212
root: "ROOT",
10182-
foobar: "FOOBAR",
10213+
baz: "BAZ",
1018310214
},
1018410215
});
1018510216
expect(t.router.state.fetchers.get(keyA)?.state).toBe("idle");
@@ -11023,7 +11054,7 @@ describe("a router", () => {
1102311054
expect(t.router.state.fetchers.get(actionKey)).toBeUndefined();
1102411055
});
1102511056

11026-
it("does not call shouldRevalidate if fetcher has no data (called 2x rapidly)", async () => {
11057+
it("does not call shouldRevalidate on POST navigation if fetcher has not yet loaded", async () => {
1102711058
// This is specifically for a Remix use case where the initial fetcher.load
1102811059
// call hasn't completed (and hasn't even loaded the route module yet), so
1102911060
// there isn't even a shouldRevalidate implementation to access yet. If
@@ -11040,7 +11071,9 @@ describe("a router", () => {
1104011071
index: true,
1104111072
},
1104211073
{
11074+
id: "page",
1104311075
path: "page",
11076+
action: true,
1104411077
},
1104511078
],
1104611079
},
@@ -11056,11 +11089,15 @@ describe("a router", () => {
1105611089
let key = "key";
1105711090
let A = await t.fetch("/fetch", key, "root");
1105811091
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
11059-
expect(A.loaders.fetch.signal.aborted).toBe(false);
1106011092

1106111093
// This should trigger an automatic revalidation of the fetcher since it
1106211094
// hasn't loaded yet
11063-
let B = await t.navigate("/page", undefined, ["fetch"]);
11095+
let B = await t.navigate(
11096+
"/page",
11097+
{ formMethod: "post", body: createFormData({}) },
11098+
["fetch"]
11099+
);
11100+
await B.actions.page.resolve("ACTION");
1106411101
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
1106511102
expect(A.loaders.fetch.signal.aborted).toBe(true);
1106611103
expect(B.loaders.fetch.signal.aborted).toBe(false);
@@ -11078,6 +11115,54 @@ describe("a router", () => {
1107811115
});
1107911116
expect(spy).not.toHaveBeenCalled();
1108011117
});
11118+
11119+
it("does not trigger revalidation on GET navigation if fetcher has not yet loaded", async () => {
11120+
let spy = jest.fn(() => true);
11121+
let t = setup({
11122+
routes: [
11123+
{
11124+
id: "root",
11125+
path: "/",
11126+
children: [
11127+
{
11128+
index: true,
11129+
},
11130+
{
11131+
id: "page",
11132+
path: "page",
11133+
loader: true,
11134+
},
11135+
],
11136+
},
11137+
{
11138+
id: "fetch",
11139+
path: "/fetch",
11140+
loader: true,
11141+
shouldRevalidate: spy,
11142+
},
11143+
],
11144+
});
11145+
11146+
let key = "key";
11147+
let A = await t.fetch("/fetch", key, "root");
11148+
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
11149+
11150+
let B = await t.navigate("/page");
11151+
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
11152+
expect(A.loaders.fetch.signal.aborted).toBe(false);
11153+
11154+
await A.loaders.fetch.resolve("A");
11155+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
11156+
11157+
// Complete the navigation
11158+
await B.loaders.page.resolve("PAGE");
11159+
expect(t.router.state.navigation.state).toBe("idle");
11160+
expect(t.router.state.fetchers.get(key)).toMatchObject({
11161+
state: "idle",
11162+
data: "A",
11163+
});
11164+
expect(spy).not.toHaveBeenCalled();
11165+
});
1108111166
});
1108211167

1108311168
describe("fetcher ?index params", () => {

‎packages/router/router.ts

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3398,7 +3398,9 @@ function getMatchesToLoad(
33983398
let fetcherMatches = matchRoutes(routesToUse, f.path, basename);
33993399

34003400
// If the fetcher path no longer matches, push it in with null matches so
3401-
// we can trigger a 404 in callLoadersAndMaybeResolveData
3401+
// we can trigger a 404 in callLoadersAndMaybeResolveData. Note this is
3402+
// currently only a use-case for Remix HMR where the route tree can change
3403+
// at runtime and remove a route previously loaded via a fetcher
34023404
if (!fetcherMatches) {
34033405
revalidatingFetchers.push({
34043406
key,
@@ -3412,28 +3414,31 @@ function getMatchesToLoad(
34123414
}
34133415

34143416
// Revalidating fetchers are decoupled from the route matches since they
3415-
// load from a static href. They only set `defaultShouldRevalidate` on
3416-
// explicit revalidation due to submission, useRevalidator, or X-Remix-Revalidate
3417-
//
3418-
// They automatically revalidate without even calling shouldRevalidate if:
3419-
// - They were cancelled
3420-
// - They're in the middle of their first load and therefore this is still
3421-
// an initial load and not a revalidation
3422-
//
3423-
// If neither of those is true, then they _always_ check shouldRevalidate
3417+
// load from a static href. They revalidate based on explicit revalidation
3418+
// (submission, useRevalidator, or X-Remix-Revalidate)
34243419
let fetcher = state.fetchers.get(key);
3425-
let isPerformingInitialLoad =
3420+
let fetcherMatch = getTargetMatch(fetcherMatches, f.path);
3421+
3422+
let shouldRevalidate = false;
3423+
if (fetchRedirectIds.has(key)) {
3424+
// Never trigger a revalidation of an actively redirecting fetcher
3425+
shouldRevalidate = false;
3426+
} else if (cancelledFetcherLoads.includes(key)) {
3427+
// Always revalidate if the fetcher was cancelled
3428+
shouldRevalidate = true;
3429+
} else if (
34263430
fetcher &&
34273431
fetcher.state !== "idle" &&
3428-
fetcher.data === undefined &&
3429-
// If a fetcher.load redirected then it'll be "loading" without any data
3430-
// so ensure we're not processing the redirect from this fetcher
3431-
!fetchRedirectIds.has(key);
3432-
let fetcherMatch = getTargetMatch(fetcherMatches, f.path);
3433-
let shouldRevalidate =
3434-
cancelledFetcherLoads.includes(key) ||
3435-
isPerformingInitialLoad ||
3436-
shouldRevalidateLoader(fetcherMatch, {
3432+
fetcher.data === undefined
3433+
) {
3434+
// If the fetcher hasn't ever completed loading yet, then this isn't a
3435+
// revalidation, it would just be a brand new load if an explicit
3436+
// revalidation is required
3437+
shouldRevalidate = isRevalidationRequired;
3438+
} else {
3439+
// Otherwise fall back on any user-defined shouldRevalidate, defaulting
3440+
// to explicit revalidations only
3441+
shouldRevalidate = shouldRevalidateLoader(fetcherMatch, {
34373442
currentUrl,
34383443
currentParams: state.matches[state.matches.length - 1].params,
34393444
nextUrl,
@@ -3442,6 +3447,7 @@ function getMatchesToLoad(
34423447
actionResult,
34433448
defaultShouldRevalidate: isRevalidationRequired,
34443449
});
3450+
}
34453451

34463452
if (shouldRevalidate) {
34473453
revalidatingFetchers.push({

0 commit comments

Comments
 (0)