From 48df1b1153d8f57811ac4d54d80d07d2bf16001e Mon Sep 17 00:00:00 2001 From: Louis Young Date: Tue, 11 Jul 2023 09:30:20 +0100 Subject: [PATCH 1/5] fix(react-router-dom): Fix `usePrompt` invalid blocker state transition - Remove the `setTimeout` wrapping `blocker.proceed`. This defers the execution of the function, but I don't think that's what we want here. - Reorder the `useEffect`s so that we attempt to proceed first when appropriate and prevent resetting a blocker that we're about to proceed. --- packages/react-router-dom/index.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index d10b1e3d7e..cf12bca451 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1461,22 +1461,22 @@ function usePageHide( function usePrompt({ when, message }: { when: boolean; message: string }) { let blocker = useBlocker(when); - React.useEffect(() => { - if (blocker.state === "blocked" && !when) { - blocker.reset(); - } - }, [blocker, when]); - React.useEffect(() => { if (blocker.state === "blocked") { let proceed = window.confirm(message); if (proceed) { - setTimeout(blocker.proceed, 0); + blocker.proceed(); } else { blocker.reset(); } } }, [blocker, message]); + + React.useEffect(() => { + if (blocker.state === "blocked" && !when) { + blocker.reset(); + } + }, [blocker, when]); } export { usePrompt as unstable_usePrompt }; From c4c96cf962582493ed3c233103cbf8dc999538e3 Mon Sep 17 00:00:00 2001 From: Louis Young Date: Tue, 11 Jul 2023 09:48:03 +0100 Subject: [PATCH 2/5] chore(CLA): Sign CLA --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index 69cdb30698..47a2659440 100644 --- a/contributors.yml +++ b/contributors.yml @@ -223,3 +223,4 @@ - yuleicul - zheng-chuang - istarkov +- louis-young From 9d708b54a4624e10e14374b2705a4b64f3f675e4 Mon Sep 17 00:00:00 2001 From: Louis Young Date: Wed, 12 Jul 2023 10:44:08 +0100 Subject: [PATCH 3/5] test(usePrompt): Write unit test suite --- .../__tests__/use-prompt-test.tsx | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 packages/react-router-dom/__tests__/use-prompt-test.tsx diff --git a/packages/react-router-dom/__tests__/use-prompt-test.tsx b/packages/react-router-dom/__tests__/use-prompt-test.tsx new file mode 100644 index 0000000000..c0f1afb804 --- /dev/null +++ b/packages/react-router-dom/__tests__/use-prompt-test.tsx @@ -0,0 +1,151 @@ +import * as React from "react"; +import { act, render, screen } from "@testing-library/react"; +import { + Link, + RouterProvider, + createBrowserRouter, + unstable_usePrompt as usePrompt, +} from "../index"; +import "@testing-library/jest-dom"; + +const PromptRoute = ({ when, message }: Parameters[0]) => { + usePrompt({ when, message }); + + return ( + <> +

Prompt Route

+ + Navigate to arbitrary route + + ); +}; + +const ArbitraryRoute = () => { + return

Arbitrary Route

; +}; + +describe("usePrompt", () => { + afterEach(() => { + jest.clearAllMocks(); + + window.history.pushState({}, "", "/"); + }); + + describe("when navigation is blocked", () => { + it("shows the confirmation prompt and does not navigate when the confirmation prompt is cancelled", () => { + const when = true; + const message = "__MESSAGE__"; + + const router = createBrowserRouter([ + { + path: "/", + element: , + }, + { + path: "/arbitrary", + element: , + }, + ]); + + render(); + + expect( + screen.getByRole("heading", { name: "Prompt Route" }) + ).toBeInTheDocument(); + + const windowConfirmMock = jest + .spyOn(window, "confirm") + .mockImplementationOnce(() => false); + + act(() => { + screen + .getByRole("link", { name: "Navigate to arbitrary route" }) + .click(); + }); + + expect(windowConfirmMock).toHaveBeenNthCalledWith(1, message); + + expect( + screen.getByRole("heading", { name: "Prompt Route" }) + ).toBeInTheDocument(); + }); + + it("shows the confirmation prompt and navigates when the confirmation prompt is accepted", () => { + const when = true; + const message = "__MESSAGE__"; + + const router = createBrowserRouter([ + { + path: "/", + element: , + }, + { + path: "/arbitrary", + element: , + }, + ]); + + render(); + + expect( + screen.getByRole("heading", { name: "Prompt Route" }) + ).toBeInTheDocument(); + + const windowConfirmMock = jest + .spyOn(window, "confirm") + .mockImplementationOnce(() => true); + + act(() => { + screen + .getByRole("link", { name: "Navigate to arbitrary route" }) + .click(); + }); + + expect(windowConfirmMock).toHaveBeenNthCalledWith(1, message); + + expect( + screen.getByRole("heading", { name: "Arbitrary Route" }) + ).toBeInTheDocument(); + }); + }); + + describe("when navigation is not blocked", () => { + it("navigates without showing the confirmation prompt", () => { + const when = false; + const message = "__MESSAGE__"; + + const router = createBrowserRouter([ + { + path: "/", + element: , + }, + { + path: "/arbitrary", + element: , + }, + ]); + + render(); + + expect( + screen.getByRole("heading", { name: "Prompt Route" }) + ).toBeInTheDocument(); + + const windowConfirmMock = jest + .spyOn(window, "confirm") + .mockImplementationOnce(() => false); + + act(() => { + screen + .getByRole("link", { name: "Navigate to arbitrary route" }) + .click(); + }); + + expect(windowConfirmMock).not.toHaveBeenCalled(); + + expect( + screen.getByRole("heading", { name: "Arbitrary Route" }) + ).toBeInTheDocument(); + }); + }); +}); From d470e998c8093ce6daf472f9ef15fc6457986a75 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 13 Jul 2023 09:46:08 -0400 Subject: [PATCH 4/5] Update tests --- .../__tests__/use-prompt-test.tsx | 195 ++++++++---------- 1 file changed, 85 insertions(+), 110 deletions(-) diff --git a/packages/react-router-dom/__tests__/use-prompt-test.tsx b/packages/react-router-dom/__tests__/use-prompt-test.tsx index c0f1afb804..64d8ce238f 100644 --- a/packages/react-router-dom/__tests__/use-prompt-test.tsx +++ b/packages/react-router-dom/__tests__/use-prompt-test.tsx @@ -1,5 +1,5 @@ import * as React from "react"; -import { act, render, screen } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import { Link, RouterProvider, @@ -7,145 +7,120 @@ import { unstable_usePrompt as usePrompt, } from "../index"; import "@testing-library/jest-dom"; - -const PromptRoute = ({ when, message }: Parameters[0]) => { - usePrompt({ when, message }); - - return ( - <> -

Prompt Route

- - Navigate to arbitrary route - - ); -}; - -const ArbitraryRoute = () => { - return

Arbitrary Route

; -}; +import { JSDOM } from "jsdom"; describe("usePrompt", () => { afterEach(() => { jest.clearAllMocks(); - - window.history.pushState({}, "", "/"); }); describe("when navigation is blocked", () => { - it("shows the confirmation prompt and does not navigate when the confirmation prompt is cancelled", () => { - const when = true; - const message = "__MESSAGE__"; - - const router = createBrowserRouter([ - { - path: "/", - element: , - }, - { - path: "/arbitrary", - element: , - }, - ]); - - render(); - - expect( - screen.getByRole("heading", { name: "Prompt Route" }) - ).toBeInTheDocument(); - + it("shows window.confirm and blocks navigation when it returns false", async () => { + let testWindow = getWindowImpl("/"); const windowConfirmMock = jest .spyOn(window, "confirm") .mockImplementationOnce(() => false); - act(() => { - screen - .getByRole("link", { name: "Navigate to arbitrary route" }) - .click(); - }); - - expect(windowConfirmMock).toHaveBeenNthCalledWith(1, message); - - expect( - screen.getByRole("heading", { name: "Prompt Route" }) - ).toBeInTheDocument(); - }); - - it("shows the confirmation prompt and navigates when the confirmation prompt is accepted", () => { - const when = true; - const message = "__MESSAGE__"; - - const router = createBrowserRouter([ - { - path: "/", - element: , - }, - { - path: "/arbitrary", - element: , - }, - ]); + let router = createBrowserRouter( + [ + { + path: "/", + Component() { + usePrompt({ when: true, message: "Are you sure??" }); + return Navigate; + }, + }, + { + path: "/arbitrary", + Component: () =>

Arbitrary

, + }, + ], + { window: testWindow } + ); render(); + expect(screen.getByText("Navigate")).toBeInTheDocument(); - expect( - screen.getByRole("heading", { name: "Prompt Route" }) - ).toBeInTheDocument(); + fireEvent.click(screen.getByText("Navigate")); + await new Promise((r) => setTimeout(r, 0)); + expect(windowConfirmMock).toHaveBeenNthCalledWith(1, "Are you sure??"); + expect(screen.getByText("Navigate")).toBeInTheDocument(); + }); + + it("shows window.confirm and navigates when it returns true", async () => { + let testWindow = getWindowImpl("/"); const windowConfirmMock = jest .spyOn(window, "confirm") .mockImplementationOnce(() => true); - act(() => { - screen - .getByRole("link", { name: "Navigate to arbitrary route" }) - .click(); - }); + let router = createBrowserRouter( + [ + { + path: "/", + Component() { + usePrompt({ when: true, message: "Are you sure??" }); + return Navigate; + }, + }, + { + path: "/arbitrary", + Component: () =>

Arbitrary

, + }, + ], + { window: testWindow } + ); + + render(); + expect(screen.getByText("Navigate")).toBeInTheDocument(); - expect(windowConfirmMock).toHaveBeenNthCalledWith(1, message); + fireEvent.click(screen.getByText("Navigate")); + await waitFor(() => screen.getByText("Arbitrary")); - expect( - screen.getByRole("heading", { name: "Arbitrary Route" }) - ).toBeInTheDocument(); + expect(windowConfirmMock).toHaveBeenNthCalledWith(1, "Are you sure??"); + expect(screen.getByText("Arbitrary")).toBeInTheDocument(); }); }); describe("when navigation is not blocked", () => { - it("navigates without showing the confirmation prompt", () => { - const when = false; - const message = "__MESSAGE__"; - - const router = createBrowserRouter([ - { - path: "/", - element: , - }, - { - path: "/arbitrary", - element: , - }, - ]); - - render(); - - expect( - screen.getByRole("heading", { name: "Prompt Route" }) - ).toBeInTheDocument(); - + it("navigates without showing window.confirm", async () => { + let testWindow = getWindowImpl("/"); const windowConfirmMock = jest .spyOn(window, "confirm") - .mockImplementationOnce(() => false); + .mockImplementation(() => true); + + let router = createBrowserRouter( + [ + { + path: "/", + Component() { + usePrompt({ when: false, message: "Are you sure??" }); + return Navigate; + }, + }, + { + path: "/arbitrary", + Component: () =>

Arbitrary

, + }, + ], + { window: testWindow } + ); + + render(); + expect(screen.getByText("Navigate")).toBeInTheDocument(); - act(() => { - screen - .getByRole("link", { name: "Navigate to arbitrary route" }) - .click(); - }); + fireEvent.click(screen.getByText("Navigate")); + await waitFor(() => screen.getByText("Arbitrary")); expect(windowConfirmMock).not.toHaveBeenCalled(); - - expect( - screen.getByRole("heading", { name: "Arbitrary Route" }) - ).toBeInTheDocument(); + expect(screen.getByText("Arbitrary")).toBeInTheDocument(); }); }); }); + +function getWindowImpl(initialUrl: string, isHash = false): Window { + // Need to use our own custom DOM in order to get a working history + const dom = new JSDOM(``, { url: "http://localhost/" }); + dom.window.history.replaceState(null, "", (isHash ? "#" : "") + initialUrl); + return dom.window as unknown as Window; +} From 9c14068569d2e53ac233611b8c631642199d550a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 13 Jul 2023 09:53:10 -0400 Subject: [PATCH 5/5] Add changeset --- .changeset/prompt-effect-order.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/prompt-effect-order.md diff --git a/.changeset/prompt-effect-order.md b/.changeset/prompt-effect-order.md new file mode 100644 index 0000000000..55280c255a --- /dev/null +++ b/.changeset/prompt-effect-order.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": patch +--- + +Reorder effects in `unstable_usePrompt` to avoid throwing an exception if the prompt is unblocked and a navigation is performed syncronously