Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unstable_useBlocker key issues in strict mode #10573

Merged
merged 8 commits into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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": "45 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.1 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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this increment into a side effect to be compliant with StrictMode. Fetchers probably have a similar issue but they don't need to put anything like a blocker function into an internal router cache so it doesn't end up causing any user-facing issues.


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(() => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this introduced a useEffect loop since setBlockerKey is used in the dependency array and changed in the same effect.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10650 please have a look

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 @@ -893,8 +893,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 });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid mutating the state Map directly

},
});
return;
Expand Down Expand Up @@ -991,9 +992,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 @@ -1032,7 +1032,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 @@ -1130,8 +1130,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 @@ -2378,8 +2379,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