Skip to content

Commit

Permalink
Properly handle fetcher redirects interrupted by normal navigations (#…
Browse files Browse the repository at this point in the history 
…10674)
  • Loading branch information
@brophdawg11
brophdawg11 committed Jul 6, 2023
1 parent 5cb2a1f commit 26dce23
Show file tree
Hide file tree
Showing 4 changed files with 240 additions and 18 deletions.
5 changes: 5 additions & 0 deletions .changeset/fetcher-redirect-interrupt.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Properly handle fetcher redirects interrupted by normal navigations
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
179 changes: 178 additions & 1 deletion packages/router/__tests__/router-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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", () => {
Expand Down
72 changes: 56 additions & 16 deletions packages/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
}

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

let originatingLoadId = incrementingLoadId;
let actionResult = await callLoaderOrAction(
"action",
fetchRequest,
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1962,6 +1990,7 @@ export function createRouter(init: RouterInit): Router {
);
fetchControllers.set(key, abortController);

let originatingLoadId = incrementingLoadId;
let result: DataResult = await callLoaderOrAction(
"loader",
fetchRequest,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 };
}
}
}
Expand Down

0 comments on commit 26dce23

Please sign in to comment.