From 834383b906234d71c079ff6122c8d733d51c5230 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 9 Jun 2023 10:19:21 -0400 Subject: [PATCH] Fix unstable_useBlocker key issues in strict mode (#10573) --- .changeset/blocker-key-strict-mode.md | 6 ++ .changeset/strip-blocker-basename.md | 5 ++ examples/navigation-blocking/src/app.tsx | 29 ++++++++-- package.json | 4 +- .../__tests__/use-blocker-test.tsx | 50 ++++++++++++++++ packages/react-router/lib/hooks.tsx | 57 ++++++++++++++----- packages/router/router.ts | 22 +++---- 7 files changed, 140 insertions(+), 33 deletions(-) create mode 100644 .changeset/blocker-key-strict-mode.md create mode 100644 .changeset/strip-blocker-basename.md diff --git a/.changeset/blocker-key-strict-mode.md b/.changeset/blocker-key-strict-mode.md new file mode 100644 index 0000000000..2851da4ade --- /dev/null +++ b/.changeset/blocker-key-strict-mode.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +Fix `unstable_useBlocker` key issues in `StrictMode` diff --git a/.changeset/strip-blocker-basename.md b/.changeset/strip-blocker-basename.md new file mode 100644 index 0000000000..ff12963ec0 --- /dev/null +++ b/.changeset/strip-blocker-basename.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Strip `basename` from locations provided to `unstable_useBlocker` functions to match `useLocation` diff --git a/examples/navigation-blocking/src/app.tsx b/examples/navigation-blocking/src/app.tsx index a2fa3b9978..d297391bea 100644 --- a/examples/navigation-blocking/src/app.tsx +++ b/examples/navigation-blocking/src/app.tsx @@ -1,5 +1,9 @@ import * as React from "react"; -import type { unstable_Blocker as Blocker } from "react-router-dom"; +import type { + unstable_Blocker as Blocker, + unstable_BlockerFunction as BlockerFunction, +} from "react-router-dom"; +import { useActionData } from "react-router-dom"; import { createBrowserRouter, createRoutesFromElements, @@ -79,22 +83,35 @@ function Layout() { } function ImportantForm() { + let actionData = useActionData() as { ok: boolean } | undefined; let [value, setValue] = React.useState(""); - let isBlocked = value !== ""; - let blocker = useBlocker(isBlocked); + // Allow the submission navigation to the same route to go through + let shouldBlock = React.useCallback( + ({ currentLocation, nextLocation }) => + value !== "" && currentLocation.pathname !== nextLocation.pathname, + [value] + ); + let blocker = useBlocker(shouldBlock); + + // Clean the input after a successful submission + React.useEffect(() => { + if (actionData?.ok) { + setValue(""); + } + }, [actionData]); // Reset the blocker if the user cleans the form React.useEffect(() => { - if (blocker.state === "blocked" && !isBlocked) { + if (blocker.state === "blocked" && value === "") { blocker.reset(); } - }, [blocker, isBlocked]); + }, [blocker, value]); return ( <>

Is the form dirty?{" "} - {isBlocked ? ( + {value !== "" ? ( Yes ) : ( No diff --git a/package.json b/package.json index 60efc1eb52..dee893e1c4 100644 --- a/package.json +++ b/package.json @@ -112,10 +112,10 @@ "none": "46.4 kB" }, "packages/react-router/dist/react-router.production.min.js": { - "none": "13.4 kB" + "none": "13.7 kB" }, "packages/react-router/dist/umd/react-router.production.min.js": { - "none": "15.8 kB" + "none": "16.1 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { "none": "12.3 kB" diff --git a/packages/react-router-dom/__tests__/use-blocker-test.tsx b/packages/react-router-dom/__tests__/use-blocker-test.tsx index 417540729c..9fa664c44b 100644 --- a/packages/react-router-dom/__tests__/use-blocker-test.tsx +++ b/packages/react-router-dom/__tests__/use-blocker-test.tsx @@ -5,6 +5,7 @@ import type { unstable_Blocker as Blocker, RouteObject } from "../index"; import { createMemoryRouter, json, + Link, NavLink, Outlet, RouterProvider, @@ -64,6 +65,55 @@ describe("navigation blocking with useBlocker", () => { }); }); + it("strips basename from location provided to blocker function", async () => { + let shouldBlock = jest.fn(); + router = createMemoryRouter( + [ + { + Component() { + useBlocker(shouldBlock); + return ( +

+ About + +
+ ); + }, + children: [ + { + path: "/", + element:

Home

, + }, + { + path: "/about", + element:

About

, + }, + ], + }, + ], + { + basename: "/base", + initialEntries: ["/base"], + } + ); + + act(() => { + root = ReactDOM.createRoot(node); + root.render(); + }); + + act(() => click(node.querySelector("a[href='/base/about']"))); + + expect(router.state.location.pathname).toBe("/base/about"); + expect(shouldBlock).toHaveBeenCalledWith({ + currentLocation: expect.objectContaining({ pathname: "/" }), + nextLocation: expect.objectContaining({ pathname: "/about" }), + historyAction: "PUSH", + }); + + act(() => root.unmount()); + }); + describe("on navigation", () => { describe("blocker returns false", () => { beforeEach(() => { diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 25891563a3..0135ea0bcc 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -22,6 +22,8 @@ import { matchRoutes, parsePath, resolveTo, + stripBasename, + IDLE_BLOCKER, UNSAFE_getPathContributingMatches as getPathContributingMatches, UNSAFE_warning as warning, } from "@remix-run/router"; @@ -933,30 +935,55 @@ let blockerId = 0; * cross-origin navigations. */ export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker { - let { router } = useDataRouterContext(DataRouterHook.UseBlocker); + let { router, basename } = useDataRouterContext(DataRouterHook.UseBlocker); let state = useDataRouterState(DataRouterStateHook.UseBlocker); - let [blockerKey] = React.useState(() => String(++blockerId)); + let [blockerKey, setBlockerKey] = React.useState(""); + let [blocker, setBlocker] = React.useState(IDLE_BLOCKER); let blockerFunction = React.useCallback( - (args) => { - return typeof shouldBlock === "function" - ? !!shouldBlock(args) - : !!shouldBlock; + (arg) => { + if (typeof shouldBlock !== "function") { + return !!shouldBlock; + } + if (basename === "/") { + return shouldBlock(arg); + } + + // If they provided us a function and we've got an active basename, strip + // it from the locations we expose to the user to match the behavior of + // useLocation + let { currentLocation, nextLocation, historyAction } = arg; + return shouldBlock({ + currentLocation: { + ...currentLocation, + pathname: + stripBasename(currentLocation.pathname, basename) || + currentLocation.pathname, + }, + nextLocation: { + ...nextLocation, + pathname: + stripBasename(nextLocation.pathname, basename) || + nextLocation.pathname, + }, + historyAction, + }); }, - [shouldBlock] + [basename, shouldBlock] ); - let blocker = router.getBlocker(blockerKey, blockerFunction); - - // Cleanup on unmount - React.useEffect( - () => () => router.deleteBlocker(blockerKey), - [router, blockerKey] - ); + React.useEffect(() => { + let key = String(++blockerId); + setBlocker(router.getBlocker(key, blockerFunction)); + setBlockerKey(key); + return () => router.deleteBlocker(key); + }, [router, setBlocker, setBlockerKey, blockerFunction]); // Prefer the blocker from state since DataRouterContext is memoized so this // ensures we update on blocker state updates - return state.blockers.get(blockerKey) || blocker; + return blockerKey && state.blockers.has(blockerKey) + ? state.blockers.get(blockerKey)! + : blocker; } /** diff --git a/packages/router/router.ts b/packages/router/router.ts index 1cd8f26a46..34d45e5249 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -927,8 +927,9 @@ export function createRouter(init: RouterInit): Router { init.history.go(delta); }, reset() { - deleteBlocker(blockerKey!); - updateState({ blockers: new Map(router.state.blockers) }); + let blockers = new Map(state.blockers); + blockers.set(blockerKey!, IDLE_BLOCKER); + updateState({ blockers }); }, }); return; @@ -1025,9 +1026,8 @@ export function createRouter(init: RouterInit): Router { // On a successful navigation we can assume we got through all blockers // so we can start fresh - for (let [key] of blockerFunctions) { - deleteBlocker(key); - } + let blockers = new Map(); + blockerFunctions.clear(); // Always respect the user flag. Otherwise don't reset on mutation // submission navigations unless they redirect @@ -1066,7 +1066,7 @@ export function createRouter(init: RouterInit): Router { newState.matches || state.matches ), preventScrollReset, - blockers: new Map(state.blockers), + blockers, }); // Reset stateful navigation vars @@ -1165,8 +1165,9 @@ export function createRouter(init: RouterInit): Router { navigate(to, opts); }, reset() { - deleteBlocker(blockerKey!); - updateState({ blockers: new Map(state.blockers) }); + let blockers = new Map(state.blockers); + blockers.set(blockerKey!, IDLE_BLOCKER); + updateState({ blockers }); }, }); return; @@ -2315,8 +2316,9 @@ export function createRouter(init: RouterInit): Router { `Invalid blocker state transition: ${blocker.state} -> ${newBlocker.state}` ); - state.blockers.set(key, newBlocker); - updateState({ blockers: new Map(state.blockers) }); + let blockers = new Map(state.blockers); + blockers.set(key, newBlocker); + updateState({ blockers }); } function shouldBlockNavigation({