From 33e99dd634bbf868ce78f0a99ac100dbd498ca28 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 30 Jun 2023 11:25:49 -0400 Subject: [PATCH 1/3] Fix issue with reused blockers on subsequent navigations --- .changeset/fix-reused-blocker.md | 6 ++ .../__tests__/use-blocker-test.tsx | 71 +++++++++++++++++++ packages/react-router/lib/hooks.tsx | 9 ++- packages/router/router.ts | 7 +- 4 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 .changeset/fix-reused-blocker.md diff --git a/.changeset/fix-reused-blocker.md b/.changeset/fix-reused-blocker.md new file mode 100644 index 0000000000..8efd6b157e --- /dev/null +++ b/.changeset/fix-reused-blocker.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +Fix issue with reused blockers on subsequent navigations diff --git a/packages/react-router-dom/__tests__/use-blocker-test.tsx b/packages/react-router-dom/__tests__/use-blocker-test.tsx index 4ffe9338b3..2e9795040f 100644 --- a/packages/react-router-dom/__tests__/use-blocker-test.tsx +++ b/packages/react-router-dom/__tests__/use-blocker-test.tsx @@ -158,6 +158,77 @@ describe("navigation blocking with useBlocker", () => { act(() => root.unmount()); }); + it("handles reused blocker in a layout route", async () => { + router = createMemoryRouter([ + { + Component() { + let blocker = useBlocker(true); + return ( +
+ /one + /two + +

{blocker.state}

+ {blocker.state === "blocked" ? ( + + ) : null} +
+ ); + }, + children: [ + { + path: "/", + element:

Home

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

One

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

Two

, + }, + ], + }, + ]); + + act(() => { + root = ReactDOM.createRoot(node); + root.render(); + }); + + // Start on / + expect(node.querySelector("h1")?.textContent).toBe("Home"); + expect(node.querySelector("p")?.textContent).toBe("unblocked"); + expect(node.querySelector("button")).toBeNull(); + + // Blocked navigation to /one + act(() => click(node.querySelector("a[href='/one']"))); + expect(node.querySelector("h1")?.textContent).toBe("Home"); + expect(node.querySelector("p")?.textContent).toBe("blocked"); + expect(node.querySelector("button")?.textContent).toBe("Proceed"); + + // Proceed to /one + act(() => click(node.querySelector("button"))); + expect(node.querySelector("h1")?.textContent).toBe("One"); + expect(node.querySelector("p")?.textContent).toBe("unblocked"); + expect(node.querySelector("button")).toBeNull(); + + // Blocked navigation to /two + act(() => click(node.querySelector("a[href='/two']"))); + expect(node.querySelector("h1")?.textContent).toBe("One"); + expect(node.querySelector("p")?.textContent).toBe("blocked"); + expect(node.querySelector("button")?.textContent).toBe("Proceed"); + + // Proceed to /two + act(() => click(node.querySelector("button"))); + expect(node.querySelector("h1")?.textContent).toBe("Two"); + expect(node.querySelector("p")?.textContent).toBe("unblocked"); + expect(node.querySelector("button")).toBeNull(); + + 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 c3177d8645..76c5075626 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -939,7 +939,6 @@ export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker { let state = useDataRouterState(DataRouterStateHook.UseBlocker); let [blockerKey, setBlockerKey] = React.useState(""); - let [blocker, setBlocker] = React.useState(IDLE_BLOCKER); let blockerFunction = React.useCallback( (arg) => { if (typeof shouldBlock !== "function") { @@ -986,15 +985,15 @@ export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker { // key of "". Until then we just have the IDLE_BLOCKER. React.useEffect(() => { if (blockerKey !== "") { - setBlocker(router.getBlocker(blockerKey, blockerFunction)); + router.getBlocker(blockerKey, blockerFunction); } }, [router, blockerKey, blockerFunction]); - // Prefer the blocker from state since DataRouterContext is memoized so this - // ensures we update on blocker state updates + // Prefer the blocker from `state` since not `router.state` DataRouterContext + // is memoized so this ensures we update on blocker state updates return blockerKey && state.blockers.has(blockerKey) ? state.blockers.get(blockerKey)! - : blocker; + : IDLE_BLOCKER; } /** diff --git a/packages/router/router.ts b/packages/router/router.ts index fdae5b0bbe..785ec73b2a 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1026,8 +1026,11 @@ export function createRouter(init: RouterInit): Router { // On a successful navigation we can assume we got through all blockers // so we can start fresh - let blockers = new Map(); - blockerFunctions.clear(); + let blockers = state.blockers; + if (blockers.size > 0) { + blockers = new Map(blockers); + blockers.forEach((_, k) => blockers.set(k, IDLE_BLOCKER)); + } // Always respect the user flag. Otherwise don't reset on mutation // submission navigations unless they redirect From fb7534d715937a2592c4e342357d4681e0625a20 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 30 Jun 2023 11:46:04 -0400 Subject: [PATCH 2/3] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 00636c56a0..a79dee318e 100644 --- a/package.json +++ b/package.json @@ -109,7 +109,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "46.5 kB" + "none": "46.6 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13.8 kB" From 599d2fce97d6f2c71f5fc0cb28ad282126c55967 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 30 Jun 2023 12:57:58 -0400 Subject: [PATCH 3/3] fix comment typo --- packages/react-router/lib/hooks.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 76c5075626..44a25ccc10 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -989,7 +989,7 @@ export function useBlocker(shouldBlock: boolean | BlockerFunction): Blocker { } }, [router, blockerKey, blockerFunction]); - // Prefer the blocker from `state` since not `router.state` DataRouterContext + // Prefer the blocker from `state` not `router.state` since DataRouterContext // is memoized so this ensures we update on blocker state updates return blockerKey && state.blockers.has(blockerKey) ? state.blockers.get(blockerKey)!