Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/brave-shrimps-wonder.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router": patch
---

Fix useBlocker when blocking function is quick to proceed
1 change: 1 addition & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
- Armanio
- arnassavickas
- aroyan
- Artur-
- ashusnapx
- avipatel97
- awreese
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createMemoryHistory } from "../../lib/router/history";
import { waitFor } from "@testing-library/react";
import { createBrowserHistory, createMemoryHistory } from "../../lib/router/history";
import type { Router } from "../../lib/router/router";
import { createRouter } from "../../lib/router/router";

Expand Down Expand Up @@ -461,6 +462,41 @@ describe("navigation blocking", () => {
});
});

describe("proceeds from blocked state using browser history", () => {
let fn = () => true;

// we want to navigate so that `/about` is the previous entry in the
// stack here since it has a loader that won't resolve immediately
beforeEach(async () => {
const history = createBrowserHistory();

router = createRouter({
history,
routes,
});

router.initialize();

await router.navigate("/");
await router.navigate("/about");
await router.navigate("/contact");
});

it("proceeds after quick block of back navigation", async () => {
router.getBlocker("KEY", fn);

await router.navigate(-1); // This does not really wait for the navigation to happen
await waitFor(
() => expect(router.getBlocker("KEY", fn).state).toBe("blocked"),
{ interval: 1 }
); // This awaits the navigation
router.getBlocker("KEY", fn).proceed!();
await waitFor(() =>
expect(router.state.location.pathname).toBe("/about")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this test exhibits the actual bug? I see this failure when run against dev:

● navigation blocking › proceeds from blocked state using browser history › proceeds after quick block of back navigation

    expect(received).toBe(expected) // Object.is equality

    Expected: "/about"
    Received: "/"

The reproduction for the original issue leaves you on the original blocked page (/three) but this test, without the fix, actually ends up going back 2 history locations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why it behaves differently here. My assumption was that maybe JSDOM (or whoever emulates browser navigation here) behaves differently in the case where navigation is in progress and you navigate again. I think it still covers the issue though, which is that if you navigate during navigation, you will end up somewhere else than you would expect.

If you have ideas on how to improve the test, I am all ears

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok yeah this is probably a JSDOM issue. I'm going to test this through an integration test in a real browser in remix-run/remix#9914 instead of trying to hack JSDOM into behaving correctly

);
});
});

describe("resets from blocked state", () => {
let fn = () => true;
it("gets an 'unblocked' blocker after resetting navigation", async () => {
Expand Down
15 changes: 9 additions & 6 deletions packages/react-router/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,7 @@ export function createRouter(init: RouterInit): Router {

// Flag to ignore the next history update, so we can revert the URL change on
// a POP navigation that was blocked by the user without touching router state
let ignoreNextHistoryUpdate = false;
let unblockBlockerHistoryUpdate: (() => void) | undefined = undefined;

let pendingRevalidationDfd: ReturnType<typeof createDeferred<void>> | null =
null;
Expand All @@ -1037,8 +1037,9 @@ export function createRouter(init: RouterInit): Router {
({ action: historyAction, location, delta }) => {
// Ignore this event if it was just us resetting the URL from a
// blocked POP navigation
if (ignoreNextHistoryUpdate) {
ignoreNextHistoryUpdate = false;
if (unblockBlockerHistoryUpdate) {
unblockBlockerHistoryUpdate();
unblockBlockerHistoryUpdate = undefined;
return;
}

Expand All @@ -1060,7 +1061,9 @@ export function createRouter(init: RouterInit): Router {

if (blockerKey && delta != null) {
// Restore the URL to match the current UI, but don't update router state
ignoreNextHistoryUpdate = true;
const nextHistoryUpdatePromise = new Promise<void>(
(resolve) => (unblockBlockerHistoryUpdate = resolve)
);
init.history.go(delta * -1);

// Put the blocker into a blocked state
Expand All @@ -1074,8 +1077,8 @@ export function createRouter(init: RouterInit): Router {
reset: undefined,
location,
});
// Re-do the same POP navigation we just blocked
init.history.go(delta);
// Re-do the same POP navigation we just blocked, after the url restoration is also complete
nextHistoryUpdatePromise.then(() => init.history.go(delta));
},
reset() {
let blockers = new Map(state.blockers);
Expand Down