From fdfcd894ae2ad42605ecaf84c526cf8c753a8589 Mon Sep 17 00:00:00 2001 From: gatzjames Date: Mon, 27 Jan 2025 23:28:35 +0100 Subject: [PATCH 1/6] add failing test --- integration/hook-useFetcher-test.ts | 119 ++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 integration/hook-useFetcher-test.ts diff --git a/integration/hook-useFetcher-test.ts b/integration/hook-useFetcher-test.ts new file mode 100644 index 0000000000..e5e11ce48e --- /dev/null +++ b/integration/hook-useFetcher-test.ts @@ -0,0 +1,119 @@ +import { test, expect } from "@playwright/test"; + +import { + createAppFixture, + createFixture, + js, +} from "./helpers/create-fixture.js"; +import type { Fixture, AppFixture } from "./helpers/create-fixture.js"; +import { PlaywrightFixture } from "./helpers/playwright-fixture.js"; + +test.describe("`useFetcher()` updates state properly when there are loaders with redirects", () => { + let fixture: Fixture; + let appFixture: AppFixture; + + test.beforeAll(async () => { + fixture = await createFixture({ + files: { + "app/data.ts": js` + export let dataList = ["one", "two", "three", "four"]; + export let getItemById = (id: string) => dataList.find((i) => i === id); + export let deleteItemById = (id: string) => { + dataList = dataList.filter((i) => i !== id); + } + `, + "app/routes/list.tsx": js` + import { useLoaderData, useFetcher, Link, Outlet } from "react-router"; + import { dataList } from "../data"; + + export function loader() { + return dataList; + } + + export default function Component() { + const list = useLoaderData() as string[]; + const deleteFetcher = useFetcher(); + + const deleteCurrentItem = async () => { + await deleteFetcher.submit(null, { + method: "POST", + action: '/list/one', + }); + }; + + return ( +
+ + + {deleteFetcher.state} + +
+
    + {list.map((item) => ( +
  • + {item} +
    • + ))} +
+
+ +
+
+
+ ); + } + `, + "app/routes/list.$id.tsx": js` + import { useLoaderData, redirect } from "react-router"; + import { getItemById, deleteItemById } from "../data"; + export async function action({ params }) { + deleteItemById(params.id); + + return null; + } + + export async function loader({ params }) { + const item = getItemById(params.id); + + if (!item) { + throw redirect("/list"); + } + + return item; + } + + export default function Component() { + let item = useLoaderData(); + return
ITEM {item}
; + } + `, + }, + }); + + appFixture = await createAppFixture(fixture); + }); + + test.afterAll(() => { + appFixture.close(); + }); + + test("after the action is called and the loader redirects the fetcher state is set to idle", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/list/one"); + await page.waitForLoadState("load"); + expect(page.getByTestId('fetcher_state')).toHaveText('idle') + await app.clickElement("#delete"); + expect(page.getByTestId('fetcher_state')).toHaveText('idle') + }); +}); From e9893e000b98bc94e071a5f3b24d33de3368e3e8 Mon Sep 17 00:00:00 2001 From: gatzjames Date: Mon, 27 Jan 2025 23:32:01 +0100 Subject: [PATCH 2/6] add fetcher update logic before redirects --- packages/react-router/lib/router/router.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index be0fe6ec48..865b1bf41e 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -2400,6 +2400,12 @@ export function createRouter(init: RouterInit): Router { let redirect = findRedirect(loaderResults); if (redirect) { + // Since we let revalidations complete even if the submitting fetcher was + // deleted, only put it back to idle if it hasn't been deleted + if (state.fetchers.has(key)) { + let doneFetcher = getDoneFetcher(actionResult.data); + state.fetchers.set(key, doneFetcher); + } return startRedirectNavigation( revalidationRequest, redirect.result, @@ -2414,6 +2420,14 @@ export function createRouter(init: RouterInit): Router { // fetchRedirectIds so it doesn't get revalidated on the next set of // loader executions fetchRedirectIds.add(redirect.key); + + // Since we let revalidations complete even if the submitting fetcher was + // deleted, only put it back to idle if it hasn't been deleted + if (state.fetchers.has(key)) { + let doneFetcher = getDoneFetcher(actionResult.data); + state.fetchers.set(key, doneFetcher); + } + return startRedirectNavigation( revalidationRequest, redirect.result, From a7d7798629e1cc500f67d5dfe55671a9d8857060 Mon Sep 17 00:00:00 2001 From: James Gatz Date: Tue, 28 Jan 2025 00:00:49 +0100 Subject: [PATCH 3/6] Update contributors.yml --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index 4f2c232fa2..33585f470b 100644 --- a/contributors.yml +++ b/contributors.yml @@ -103,6 +103,7 @@ - fyzhu - fz6m - gaspard +- gatzjames - gavriguy - Geist5000 - gesposito From c483d681b2179a64d30fd45917e6acdd10052c7b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 May 2025 14:02:50 -0400 Subject: [PATCH 4/6] Add test, update approach --- .../__tests__/router/fetchers-test.ts | 37 +++++++++++++++++++ packages/react-router/lib/router/router.ts | 27 ++++---------- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/packages/react-router/__tests__/router/fetchers-test.ts b/packages/react-router/__tests__/router/fetchers-test.ts index fa051a7cd0..739ea8bd46 100644 --- a/packages/react-router/__tests__/router/fetchers-test.ts +++ b/packages/react-router/__tests__/router/fetchers-test.ts @@ -841,6 +841,43 @@ describe("fetchers", () => { expect(t.router.state.location.pathname).toBe("/"); expect(t.router.state.location.key).toBe(key); }); + + test("handles loader redirects after a fetcher submission", async () => { + let t = initializeTest(); + + let A = await t.navigate("/foo"); + await A.loaders.foo.resolve("FOO"); + expect(t.router.state).toMatchObject({ + location: { pathname: "/foo" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT", foo: "FOO" }, + }); + + let key = "key"; + let B = await t.fetch("/bar", key, { + formMethod: "post", + formData: createFormData({}), + }); + await B.actions.bar.resolve("ACTION"); + expect(t.fetchers[key]).toMatchObject({ + state: "loading", + data: "ACTION", + }); + await B.loaders.root.resolve("ROOT*"); + + let C = await B.loaders.foo.redirect("/"); + await C.loaders.root.resolve("ROOT**"); + await C.loaders.index.resolve("INDEX*"); + expect(t.router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT**", index: "INDEX*" }, + }); + expect(t.fetchers[key]).toMatchObject({ + state: "idle", + data: "ACTION", + }); + }); }); describe("fetcher resubmissions/re-gets", () => { diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index 981523ca0e..1c5338ec02 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -2494,14 +2494,15 @@ export function createRouter(init: RouterInit): Router { fetchControllers.delete(key); revalidatingFetchers.forEach((r) => fetchControllers.delete(r.key)); + // Since we let revalidations complete even if the submitting fetcher was + // deleted, only put it back to idle if it hasn't been deleted + if (state.fetchers.has(key)) { + let doneFetcher = getDoneFetcher(actionResult.data); + state.fetchers.set(key, doneFetcher); + } + let redirect = findRedirect(loaderResults); if (redirect) { - // Since we let revalidations complete even if the submitting fetcher was - // deleted, only put it back to idle if it hasn't been deleted - if (state.fetchers.has(key)) { - let doneFetcher = getDoneFetcher(actionResult.data); - state.fetchers.set(key, doneFetcher); - } return startRedirectNavigation( revalidationRequest, redirect.result, @@ -2517,13 +2518,6 @@ export function createRouter(init: RouterInit): Router { // loader executions fetchRedirectIds.add(redirect.key); - // Since we let revalidations complete even if the submitting fetcher was - // deleted, only put it back to idle if it hasn't been deleted - if (state.fetchers.has(key)) { - let doneFetcher = getDoneFetcher(actionResult.data); - state.fetchers.set(key, doneFetcher); - } - return startRedirectNavigation( revalidationRequest, redirect.result, @@ -2542,13 +2536,6 @@ export function createRouter(init: RouterInit): Router { fetcherResults ); - // Since we let revalidations complete even if the submitting fetcher was - // deleted, only put it back to idle if it hasn't been deleted - if (state.fetchers.has(key)) { - let doneFetcher = getDoneFetcher(actionResult.data); - state.fetchers.set(key, doneFetcher); - } - abortStaleFetchLoads(loadId); // If we are currently in a navigation loading state and this fetcher is From 1dd055e69dac68eb991b4fdf7512627a66f797dd Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 May 2025 14:03:37 -0400 Subject: [PATCH 5/6] Updates --- integration/hook-useFetcher-test.ts | 119 --------------------- packages/react-router/lib/router/router.ts | 1 - 2 files changed, 120 deletions(-) delete mode 100644 integration/hook-useFetcher-test.ts diff --git a/integration/hook-useFetcher-test.ts b/integration/hook-useFetcher-test.ts deleted file mode 100644 index e5e11ce48e..0000000000 --- a/integration/hook-useFetcher-test.ts +++ /dev/null @@ -1,119 +0,0 @@ -import { test, expect } from "@playwright/test"; - -import { - createAppFixture, - createFixture, - js, -} from "./helpers/create-fixture.js"; -import type { Fixture, AppFixture } from "./helpers/create-fixture.js"; -import { PlaywrightFixture } from "./helpers/playwright-fixture.js"; - -test.describe("`useFetcher()` updates state properly when there are loaders with redirects", () => { - let fixture: Fixture; - let appFixture: AppFixture; - - test.beforeAll(async () => { - fixture = await createFixture({ - files: { - "app/data.ts": js` - export let dataList = ["one", "two", "three", "four"]; - export let getItemById = (id: string) => dataList.find((i) => i === id); - export let deleteItemById = (id: string) => { - dataList = dataList.filter((i) => i !== id); - } - `, - "app/routes/list.tsx": js` - import { useLoaderData, useFetcher, Link, Outlet } from "react-router"; - import { dataList } from "../data"; - - export function loader() { - return dataList; - } - - export default function Component() { - const list = useLoaderData() as string[]; - const deleteFetcher = useFetcher(); - - const deleteCurrentItem = async () => { - await deleteFetcher.submit(null, { - method: "POST", - action: '/list/one', - }); - }; - - return ( -
- - - {deleteFetcher.state} - -
-
    - {list.map((item) => ( -
  • - {item} -
    • - ))} -
-
- -
-
-
- ); - } - `, - "app/routes/list.$id.tsx": js` - import { useLoaderData, redirect } from "react-router"; - import { getItemById, deleteItemById } from "../data"; - export async function action({ params }) { - deleteItemById(params.id); - - return null; - } - - export async function loader({ params }) { - const item = getItemById(params.id); - - if (!item) { - throw redirect("/list"); - } - - return item; - } - - export default function Component() { - let item = useLoaderData(); - return
ITEM {item}
; - } - `, - }, - }); - - appFixture = await createAppFixture(fixture); - }); - - test.afterAll(() => { - appFixture.close(); - }); - - test("after the action is called and the loader redirects the fetcher state is set to idle", async ({ - page, - }) => { - let app = new PlaywrightFixture(appFixture, page); - await app.goto("/list/one"); - await page.waitForLoadState("load"); - expect(page.getByTestId('fetcher_state')).toHaveText('idle') - await app.clickElement("#delete"); - expect(page.getByTestId('fetcher_state')).toHaveText('idle') - }); -}); diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index 1c5338ec02..9ab1a05d5f 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -2517,7 +2517,6 @@ export function createRouter(init: RouterInit): Router { // fetchRedirectIds so it doesn't get revalidated on the next set of // loader executions fetchRedirectIds.add(redirect.key); - return startRedirectNavigation( revalidationRequest, redirect.result, From ce7c215159094eb76457d6be27cda6f62da2c38f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 May 2025 14:09:02 -0400 Subject: [PATCH 6/6] Add changeset --- .changeset/curvy-lobsters-accept.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/curvy-lobsters-accept.md diff --git a/.changeset/curvy-lobsters-accept.md b/.changeset/curvy-lobsters-accept.md new file mode 100644 index 0000000000..a2828b7874 --- /dev/null +++ b/.changeset/curvy-lobsters-accept.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Fix bug where a submitting `fetcher` would get stuck in a `loading` state if a revalidating `loader` redirected