Skip to content

Commit

Permalink
Fix unstable_useBlocker key issues in strict mode (#10573)
Browse files Browse the repository at this point in the history
  • Loading branch information
brophdawg11 authored Jun 9, 2023
1 parent 6acd1b0 commit 834383b
Show file tree
Hide file tree
Showing 7 changed files with 140 additions and 33 deletions.
6 changes: 6 additions & 0 deletions .changeset/blocker-key-strict-mode.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"react-router": patch
"@remix-run/router": patch
---

Fix `unstable_useBlocker` key issues in `StrictMode`
5 changes: 5 additions & 0 deletions .changeset/strip-blocker-basename.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router": patch
---

Strip `basename` from locations provided to `unstable_useBlocker` functions to match `useLocation`
29 changes: 23 additions & 6 deletions examples/navigation-blocking/src/app.tsx
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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<BlockerFunction>(
({ 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 (
<>
<p>
Is the form dirty?{" "}
{isBlocked ? (
{value !== "" ? (
<span style={{ color: "red" }}>Yes</span>
) : (
<span style={{ color: "green" }}>No</span>
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
50 changes: 50 additions & 0 deletions packages/react-router-dom/__tests__/use-blocker-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { unstable_Blocker as Blocker, RouteObject } from "../index";
import {
createMemoryRouter,
json,
Link,
NavLink,
Outlet,
RouterProvider,
Expand Down Expand Up @@ -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 (
<div>
<Link to="/about">About</Link>
<Outlet />
</div>
);
},
children: [
{
path: "/",
element: <h1>Home</h1>,
},
{
path: "/about",
element: <h1>About</h1>,
},
],
},
],
{
basename: "/base",
initialEntries: ["/base"],
}
);

act(() => {
root = ReactDOM.createRoot(node);
root.render(<RouterProvider router={router} />);
});

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 <Link> navigation", () => {
describe("blocker returns false", () => {
beforeEach(() => {
Expand Down
57 changes: 42 additions & 15 deletions packages/react-router/lib/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import {
matchRoutes,
parsePath,
resolveTo,
stripBasename,
IDLE_BLOCKER,
UNSAFE_getPathContributingMatches as getPathContributingMatches,
UNSAFE_warning as warning,
} from "@remix-run/router";
Expand Down Expand Up @@ -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<Blocker>(IDLE_BLOCKER);
let blockerFunction = React.useCallback<BlockerFunction>(
(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;
}

/**
Expand Down
22 changes: 12 additions & 10 deletions packages/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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({
Expand Down

0 comments on commit 834383b

Please sign in to comment.