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.

Sign up for GitHub

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

Jump to bottom

fix: Wait for restore url navigation to complete before proceeding #11620

Closed
wants to merge 16 commits into from
Closed

fix: Wait for restore url navigation to complete before proceeding #11620

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7771b54
fix: Wait for restore url navigation to complete before proceeding
Artur- Jun 6, 2024
10783a6
cla
Artur- Jun 6, 2024
e9ea771
simplify
Artur- Jun 6, 2024
5eaa15d
simplify
Artur- Jun 6, 2024
1914532
prettier
Artur- Jun 6, 2024
98946ee
Make tests pass
Artur- Jun 6, 2024
1ff3910
wait some more
Artur- Jun 6, 2024
2611e9c
Stabilize test by using await instead of sleep
Artur- Jun 6, 2024
412a12e
Wait for navigation to finish in all tests
Artur- Jun 10, 2024
1682389
sleep -> await
Artur- Jun 10, 2024
34c2da2
Merge remote-tracking branch 'upstream/dev' into HEAD
Artur- Jul 21, 2024
cc8e434
changeset
Artur- Jul 21, 2024
82d4344
Test for immediately proceeding blocker
Artur- Jul 23, 2024
93bc074
Merge remote-tracking branch 'origin/dev' into wait-for-restore-url
Artur- Aug 23, 2024
ff436a5
Revert tests
Artur- Aug 23, 2024
6071fb1
Rename variable
Artur- Aug 23, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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
83 changes: 36 additions & 47 deletions packages/react-router/__tests__/dom/use-blocker-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
useBlocker,
useNavigate,
} from "../../index";
import { waitFor } from "@testing-library/react";

type Router = ReturnType<typeof createMemoryRouter>;

Expand Down Expand Up @@ -279,10 +280,9 @@ describe("navigation blocking with useBlocker", () => {
it("navigates", async () => {
await act(async () => {
click(node.querySelector("a[href='/about']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("About");
await waitFor(() => expect(h1?.textContent).toBe("About"));
});

it("gets an 'unblocked' blocker after navigation starts", async () => {
Expand All @@ -300,14 +300,15 @@ describe("navigation blocking with useBlocker", () => {
it("gets an 'unblocked' blocker after navigation completes", async () => {
await act(async () => {
click(node.querySelector("a[href='/about']"));
await sleep(LOADER_LATENCY_MS);
});
expect(blocker).toEqual({
state: "unblocked",
proceed: undefined,
reset: undefined,
location: undefined,
});
await waitFor(() =>
expect(blocker).toEqual({
state: "unblocked",
proceed: undefined,
reset: undefined,
location: undefined,
})
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we revert these unrelated changes to the tests and do them in a standalone PR? I'm not against changing the approach but I'd prefer to keep the functional bug fix changes separate from the unrelated test cleanup changes. Bug fix PRs like this are, IMO, best when they are 1 or more net-new tests demonstrating the bug and the code changes to make those test pass

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I really do not want to make any changes to any other tests but this was needed, at least originally, to get tests to pass with the fix. I now reverted all these changes and included one commit with the test that fails in current dev and another commit with the fix. Let's see if tests pass or not, and if not, I can create a separate PR first with the test changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok let me take a look at those locally. I noticed this was off dev too but we'll need it to be off v6 if we want to release a fix in 6.x so I'll probably re-open a new PR against v6 and will include you as a coauthor on that one

});
});

Expand Down Expand Up @@ -485,10 +486,9 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='proceed']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("About");
await waitFor(() => expect(h1?.textContent).toBe("About"));
});

it("gets an 'unblocked' blocker after resetting navigation", async () => {
Expand All @@ -512,11 +512,9 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='reset']"));
// wait for '/about' loader so we catch failure if navigation proceeds
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("Home");
await waitFor(() => expect(h1?.textContent).toBe("Home"));
});
});
});
Expand Down Expand Up @@ -575,10 +573,9 @@ describe("navigation blocking with useBlocker", () => {
it("navigates", async () => {
await act(async () => {
click(node.querySelector("a[href='/about']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("About");
await waitFor(() => expect(h1?.textContent).toBe("About"));
});

it("gets an 'unblocked' blocker after navigation starts", async () => {
Expand Down Expand Up @@ -660,10 +657,9 @@ describe("navigation blocking with useBlocker", () => {
it("does not navigate", async () => {
await act(async () => {
click(node.querySelector("a[href='/about']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).not.toBe("About");
await waitFor(() => expect(h1?.textContent).not.toBe("About"));
});

it("gets a 'blocked' blocker after navigation starts", async () => {
Expand Down Expand Up @@ -773,14 +769,15 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='proceed']"));
await sleep(LOADER_LATENCY_MS);
});
expect(blocker).toEqual({
state: "unblocked",
proceed: undefined,
reset: undefined,
location: undefined,
});
await waitFor(() =>
expect(blocker).toEqual({
state: "unblocked",
proceed: undefined,
reset: undefined,
location: undefined,
})
);
});

it("navigates after proceeding navigation completes", async () => {
Expand All @@ -789,10 +786,9 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='proceed']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("About");
await waitFor(() => expect(h1?.textContent).toBe("About"));
});

it("gets an 'unblocked' blocker after resetting navigation", async () => {
Expand All @@ -816,11 +812,9 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='reset']"));
// wait for '/about' loader so we catch failure if navigation proceeds
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("Home");
await waitFor(() => expect(h1?.textContent).toBe("Home"));
});
});
});
Expand Down Expand Up @@ -887,10 +881,9 @@ describe("navigation blocking with useBlocker", () => {
it("navigates", async () => {
await act(async () => {
click(node.querySelector("[data-action='back']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("About");
await waitFor(() => expect(h1?.textContent).toBe("About"));
});

it("gets an 'unblocked' blocker after navigation starts", async () => {
Expand All @@ -908,7 +901,6 @@ describe("navigation blocking with useBlocker", () => {
it("gets an 'unblocked' blocker after navigation completes", async () => {
await act(async () => {
click(node.querySelector("[data-action='back']"));
await sleep(LOADER_LATENCY_MS);
});
expect(blocker).toEqual({
state: "unblocked",
Expand Down Expand Up @@ -980,10 +972,9 @@ describe("navigation blocking with useBlocker", () => {
it("does not navigate", async () => {
await act(async () => {
click(node.querySelector("[data-action='back']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).not.toBe("About");
await waitFor(() => expect(h1?.textContent).not.toBe("About"));
});

it("gets a 'blocked' blocker after navigation starts", async () => {
Expand All @@ -1001,7 +992,6 @@ describe("navigation blocking with useBlocker", () => {
it("gets a 'blocked' blocker after navigation promise resolves", async () => {
await act(async () => {
click(node.querySelector("[data-action='back']"));
await sleep(LOADER_LATENCY_MS);
});
expect(blocker).toEqual({
state: "blocked",
Expand Down Expand Up @@ -1101,14 +1091,15 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='proceed']"));
await sleep(LOADER_LATENCY_MS);
});
expect(blocker).toEqual({
state: "unblocked",
proceed: undefined,
reset: undefined,
location: undefined,
});
await waitFor(() =>
expect(blocker).toEqual({
state: "unblocked",
proceed: undefined,
reset: undefined,
location: undefined,
})
);
});

it("navigates after proceeding navigation completes", async () => {
Expand All @@ -1117,10 +1108,9 @@ describe("navigation blocking with useBlocker", () => {
});
await act(async () => {
click(node.querySelector("[data-action='proceed']"));
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("About");
await waitFor(() => expect(h1?.textContent).toBe("About"));
});

it("gets an 'unblocked' blocker after resetting navigation", async () => {
Expand All @@ -1145,10 +1135,9 @@ describe("navigation blocking with useBlocker", () => {
await act(async () => {
click(node.querySelector("[data-action='reset']"));
// wait for '/about' loader so we catch failure if navigation proceeds
await sleep(LOADER_LATENCY_MS);
});
let h1 = node.querySelector("h1");
expect(h1?.textContent).toBe("Contact");
await waitFor(() => expect(h1?.textContent).toBe("Contact"));
});
});
});
Expand Down
Loading