Skip to content

Commit 26dce23

Browse files
brophdawg11brophdawg11
authored
Properly handle fetcher redirects interrupted by normal navigations (#10674)
1 parent 5cb2a1f commit 26dce23

File tree

4 files changed

+240
-18
lines changed

4 files changed

+240
-18
lines changed

‎.changeset/fetcher-redirect-interrupt.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+
Properly handle fetcher redirects interrupted by normal navigations

‎package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@
109109
},
110110
"filesize": {
111111
"packages/router/dist/router.umd.min.js": {
112-
"none": "46.6 kB"
112+
"none": "46.9 kB"
113113
},
114114
"packages/react-router/dist/react-router.production.min.js": {
115115
"none": "13.8 kB"

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

Lines changed: 178 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9726,7 +9726,10 @@ describe("a router", () => {
97269726
expect(A.loaders.foo.signal.aborted).toBe(false);
97279727
expect(t.router.state.navigation.state).toBe("idle");
97289728
expect(t.router.state.location.pathname).toBe("/foo");
9729-
expect(t.router.state.loaderData.foo).toBe("B");
9729+
expect(t.router.state.loaderData).toEqual({
9730+
root: "B root",
9731+
foo: "B",
9732+
});
97309733

97319734
await A.loaders.root.resolve("A root");
97329735
await A.loaders.foo.resolve("A");
@@ -10009,6 +10012,180 @@ describe("a router", () => {
1000910012
expect(t.router.state.fetchers.get(key)?.data).toBeUndefined();
1001010013
});
1001110014
});
10015+
10016+
describe(`
10017+
A) fetch POST /foo |--R
10018+
B) nav GET /bar |---O
10019+
`, () => {
10020+
it("ignores submission redirect navigation if preceded by a normal navigation", async () => {
10021+
let key = "key";
10022+
let t = initializeTmTest();
10023+
let A = await t.fetch("/foo", key, {
10024+
formMethod: "post",
10025+
formData: createFormData({ key: "value" }),
10026+
});
10027+
let B = await t.navigate("/bar");
10028+
10029+
// This redirect should be ignored
10030+
await A.actions.foo.redirect("/baz");
10031+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
10032+
10033+
await B.loaders.root.resolve("ROOT*");
10034+
await B.loaders.bar.resolve("BAR");
10035+
expect(t.router.state).toMatchObject({
10036+
navigation: IDLE_NAVIGATION,
10037+
location: { pathname: "/bar" },
10038+
loaderData: {
10039+
root: "ROOT*",
10040+
bar: "BAR",
10041+
},
10042+
});
10043+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
10044+
expect(t.router.state.fetchers.get(key)?.data).toBeUndefined();
10045+
});
10046+
});
10047+
10048+
describe(`
10049+
A) fetch GET /foo |--R
10050+
B) nav GET /bar |---O
10051+
`, () => {
10052+
it("ignores loader redirect navigation if preceded by a normal navigation", async () => {
10053+
let key = "key";
10054+
let t = initializeTmTest();
10055+
10056+
// Start a fetch load and interrupt with a navigation
10057+
let A = await t.fetch("/foo", key);
10058+
let B = await t.navigate("/bar", undefined, ["foo"]);
10059+
10060+
// The fetcher loader redirect should be ignored
10061+
await A.loaders.foo.redirect("/baz");
10062+
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
10063+
10064+
// The navigation should trigger the fetcher to revalidate since it's
10065+
// not yet "completed". If it returns data this time that should be
10066+
// reflected
10067+
await B.loaders.bar.resolve("BAR");
10068+
await B.loaders.foo.resolve("FOO");
10069+
10070+
expect(t.router.state).toMatchObject({
10071+
navigation: IDLE_NAVIGATION,
10072+
location: { pathname: "/bar" },
10073+
loaderData: {
10074+
root: "ROOT",
10075+
bar: "BAR",
10076+
},
10077+
});
10078+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
10079+
expect(t.router.state.fetchers.get(key)?.data).toBe("FOO");
10080+
});
10081+
10082+
it("processes second fetcher load redirect after interruption by normal navigation", async () => {
10083+
let key = "key";
10084+
let t = initializeTmTest();
10085+
10086+
// Start a fetch load and interrupt with a navigation
10087+
let A = await t.fetch("/foo", key, "root");
10088+
let B = await t.navigate("/bar", undefined, ["foo"]);
10089+
10090+
// The fetcher loader redirect should be ignored
10091+
await A.loaders.foo.redirect("/baz");
10092+
expect(t.router.state).toMatchObject({
10093+
navigation: { location: { pathname: "/bar" } },
10094+
location: { pathname: "/" },
10095+
});
10096+
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
10097+
10098+
// The navigation should trigger the fetcher to revalidate since it's
10099+
// not yet "completed". If it redirects again we should follow that
10100+
await B.loaders.bar.resolve("BAR");
10101+
let C = await B.loaders.foo.redirect(
10102+
"/foo/bar",
10103+
undefined,
10104+
undefined,
10105+
["foo"]
10106+
);
10107+
expect(t.router.state).toMatchObject({
10108+
navigation: { location: { pathname: "/foo/bar" } },
10109+
location: { pathname: "/" },
10110+
loaderData: {
10111+
root: "ROOT",
10112+
},
10113+
});
10114+
expect(t.router.state.fetchers.get(key)?.state).toBe("loading");
10115+
10116+
// The fetcher should not revalidate here since it triggered the redirect
10117+
await C.loaders.foobar.resolve("FOOBAR");
10118+
expect(t.router.state).toMatchObject({
10119+
navigation: IDLE_NAVIGATION,
10120+
location: { pathname: "/foo/bar" },
10121+
loaderData: {
10122+
root: "ROOT",
10123+
foobar: "FOOBAR",
10124+
},
10125+
});
10126+
expect(t.router.state.fetchers.get(key)?.state).toBe("idle");
10127+
expect(t.router.state.fetchers.get(key)?.data).toBe(undefined);
10128+
});
10129+
10130+
it("handle multiple fetcher loader redirects", async () => {
10131+
let keyA = "a";
10132+
let keyB = "b";
10133+
let t = initializeTmTest();
10134+
10135+
// Start 2 fetch loads
10136+
let A = await t.fetch("/foo", keyA, "root");
10137+
let B = await t.fetch("/bar", keyB, "root");
10138+
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+
]);
10144+
expect(t.router.state).toMatchObject({
10145+
navigation: { location: { pathname: "/baz" } },
10146+
location: { pathname: "/" },
10147+
});
10148+
expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading");
10149+
expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading");
10150+
10151+
// The original fetch load redirect should be ignored
10152+
await A.loaders.foo.redirect("/foo/bar");
10153+
expect(t.router.state).toMatchObject({
10154+
navigation: { location: { pathname: "/baz" } },
10155+
location: { pathname: "/" },
10156+
});
10157+
expect(t.router.state.fetchers.get(keyA)?.state).toBe("loading");
10158+
expect(t.router.state.fetchers.get(keyB)?.state).toBe("loading");
10159+
10160+
// Resolve the navigation loader and the revalidating (first) fetcher
10161+
// loader which redirects again
10162+
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");
10177+
expect(t.router.state).toMatchObject({
10178+
navigation: IDLE_NAVIGATION,
10179+
location: { pathname: "/foo/bar" },
10180+
loaderData: {
10181+
root: "ROOT",
10182+
foobar: "FOOBAR",
10183+
},
10184+
});
10185+
expect(t.router.state.fetchers.get(keyA)?.state).toBe("idle");
10186+
expect(t.router.state.fetchers.get(keyB)?.state).toBe("idle");
10187+
});
10188+
});
1001210189
});
1001310190

1001410191
describe("fetcher revalidation", () => {

‎packages/router/router.ts

Lines changed: 56 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1591,7 +1591,15 @@ export function createRouter(init: RouterInit): Router {
15911591
// If any loaders returned a redirect Response, start a new REPLACE navigation
15921592
let redirect = findRedirect(results);
15931593
if (redirect) {
1594-
await startRedirectNavigation(state, redirect, { replace });
1594+
if (redirect.idx >= matchesToLoad.length) {
1595+
// If this redirect came from a fetcher make sure we mark it in
1596+
// fetchRedirectIds so it doesn't get revalidated on the next set of
1597+
// loader executions
1598+
let fetcherKey =
1599+
revalidatingFetchers[redirect.idx - matchesToLoad.length].key;
1600+
fetchRedirectIds.add(fetcherKey);
1601+
}
1602+
await startRedirectNavigation(state, redirect.result, { replace });
15951603
return { shortCircuited: true };
15961604
}
15971605

@@ -1739,6 +1747,7 @@ export function createRouter(init: RouterInit): Router {
17391747
);
17401748
fetchControllers.set(key, abortController);
17411749

1750+
let originatingLoadId = incrementingLoadId;
17421751
let actionResult = await callLoaderOrAction(
17431752
"action",
17441753
fetchRequest,
@@ -1760,15 +1769,26 @@ export function createRouter(init: RouterInit): Router {
17601769

17611770
if (isRedirectResult(actionResult)) {
17621771
fetchControllers.delete(key);
1763-
fetchRedirectIds.add(key);
1764-
let loadingFetcher = getLoadingFetcher(submission);
1765-
state.fetchers.set(key, loadingFetcher);
1766-
updateState({ fetchers: new Map(state.fetchers) });
1767-
1768-
return startRedirectNavigation(state, actionResult, {
1769-
submission,
1770-
isFetchActionRedirect: true,
1771-
});
1772+
if (pendingNavigationLoadId > originatingLoadId) {
1773+
// A new navigation was kicked off after our action started, so that
1774+
// should take precedence over this redirect navigation. We already
1775+
// set isRevalidationRequired so all loaders for the new route should
1776+
// fire unless opted out via shouldRevalidate
1777+
let doneFetcher = getDoneFetcher(undefined);
1778+
state.fetchers.set(key, doneFetcher);
1779+
updateState({ fetchers: new Map(state.fetchers) });
1780+
return;
1781+
} else {
1782+
fetchRedirectIds.add(key);
1783+
let loadingFetcher = getLoadingFetcher(submission);
1784+
state.fetchers.set(key, loadingFetcher);
1785+
updateState({ fetchers: new Map(state.fetchers) });
1786+
1787+
return startRedirectNavigation(state, actionResult, {
1788+
submission,
1789+
isFetchActionRedirect: true,
1790+
});
1791+
}
17721792
}
17731793

17741794
// Process any non-redirect errors thrown
@@ -1875,7 +1895,15 @@ export function createRouter(init: RouterInit): Router {
18751895

18761896
let redirect = findRedirect(results);
18771897
if (redirect) {
1878-
return startRedirectNavigation(state, redirect);
1898+
if (redirect.idx >= matchesToLoad.length) {
1899+
// If this redirect came from a fetcher make sure we mark it in
1900+
// fetchRedirectIds so it doesn't get revalidated on the next set of
1901+
// loader executions
1902+
let fetcherKey =
1903+
revalidatingFetchers[redirect.idx - matchesToLoad.length].key;
1904+
fetchRedirectIds.add(fetcherKey);
1905+
}
1906+
return startRedirectNavigation(state, redirect.result);
18791907
}
18801908

18811909
// Process and commit output from loaders
@@ -1962,6 +1990,7 @@ export function createRouter(init: RouterInit): Router {
19621990
);
19631991
fetchControllers.set(key, abortController);
19641992

1993+
let originatingLoadId = incrementingLoadId;
19651994
let result: DataResult = await callLoaderOrAction(
19661995
"loader",
19671996
fetchRequest,
@@ -1994,9 +2023,18 @@ export function createRouter(init: RouterInit): Router {
19942023

19952024
// If the loader threw a redirect Response, start a new REPLACE navigation
19962025
if (isRedirectResult(result)) {
1997-
fetchRedirectIds.add(key);
1998-
await startRedirectNavigation(state, result);
1999-
return;
2026+
if (pendingNavigationLoadId > originatingLoadId) {
2027+
// A new navigation was kicked off after our loader started, so that
2028+
// should take precedence over this redirect navigation
2029+
let doneFetcher = getDoneFetcher(undefined);
2030+
state.fetchers.set(key, doneFetcher);
2031+
updateState({ fetchers: new Map(state.fetchers) });
2032+
return;
2033+
} else {
2034+
fetchRedirectIds.add(key);
2035+
await startRedirectNavigation(state, result);
2036+
return;
2037+
}
20002038
}
20012039

20022040
// Process any non-redirect errors thrown
@@ -4090,11 +4128,13 @@ function getInternalRouterError(
40904128
}
40914129

40924130
// Find any returned redirect errors, starting from the lowest match
4093-
function findRedirect(results: DataResult[]): RedirectResult | undefined {
4131+
function findRedirect(
4132+
results: DataResult[]
4133+
): { result: RedirectResult; idx: number } | undefined {
40944134
for (let i = results.length - 1; i >= 0; i--) {
40954135
let result = results[i];
40964136
if (isRedirectResult(result)) {
4097-
return result;
4137+
return { result, idx: i };
40984138
}
40994139
}
41004140
}

0 commit comments

Comments
 (0)