From 0d8e6d48847641fbd23d46aa62d362b1216ff2fb Mon Sep 17 00:00:00 2001
From: 42shadow42 <42shadow42@gmail.com>
Date: Tue, 25 Apr 2023 08:18:13 -0500
Subject: [PATCH 1/7] Allow navigation in child useEffects and fix intermittent
issue where browser automation fails to navigate due to race conditions.
(#8929)
---
.../__tests__/useNavigate-test.tsx | 71 +++++++++++++++++++
packages/react-router/lib/hooks.tsx | 4 +-
2 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx
index cd9d9d2433..e0fb652665 100644
--- a/packages/react-router/__tests__/useNavigate-test.tsx
+++ b/packages/react-router/__tests__/useNavigate-test.tsx
@@ -167,6 +167,77 @@ describe("useNavigate", () => {
]
`);
});
+
+ it("transitions to the new location when called immediately", () => {
+ const Home = React.forwardRef(function Home(_props, ref) {
+ let navigate = useNavigate();
+
+ React.useImperativeHandle(ref, () => ({
+ navigate: () => navigate("/about")
+ }))
+
+ return null
+ })
+
+ let homeRef;
+
+ let renderer: TestRenderer.ReactTestRenderer;
+ renderer = TestRenderer.create(
+
+
+ homeRef = ref} />} />
+ About} />
+
+
+ );
+
+ TestRenderer.act(() => {
+ homeRef.navigate();
+ })
+
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ About
+
+ `);
+ });
+
+ it("allows navigation in child useEffects", () => {
+ function Child({ onChildRendered }) {
+
+ React.useEffect(() => {
+ onChildRendered();
+ });
+
+ return null;
+ }
+
+ function Parent() {
+ let navigate = useNavigate();
+
+ let onChildRendered = React.useCallback(() => navigate("/about"), []);
+
+ return ;
+ }
+
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create(
+
+
+ } />
+ About} />
+
+
+ );
+ });
+
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ About
+
+ `);
+ });
it("navigates to the new location with empty query string when no query string is provided", () => {
function Home() {
diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx
index e21e533195..91e16a16ec 100644
--- a/packages/react-router/lib/hooks.tsx
+++ b/packages/react-router/lib/hooks.tsx
@@ -180,7 +180,7 @@ function useNavigateUnstable(): NavigateFunction {
);
let activeRef = React.useRef(false);
- React.useEffect(() => {
+ React.useLayoutEffect(() => {
activeRef.current = true;
});
@@ -192,8 +192,6 @@ function useNavigateUnstable(): NavigateFunction {
`your component is first rendered.`
);
- if (!activeRef.current) return;
-
if (typeof to === "number") {
navigator.go(to);
return;
From 92284dcddd0ae9a4ba2410990c76e7c960cb32e2 Mon Sep 17 00:00:00 2001
From: Matt Brophy
Date: Tue, 25 Apr 2023 10:08:20 -0400
Subject: [PATCH 2/7] Add tests and switch to useIsomorphicLayoutEffect
---
.changeset/navigate-in-effect.md | 5 +
.../__tests__/useNavigate-test.tsx | 273 +++++++++++++-----
packages/react-router/lib/hooks.tsx | 38 ++-
3 files changed, 239 insertions(+), 77 deletions(-)
create mode 100644 .changeset/navigate-in-effect.md
diff --git a/.changeset/navigate-in-effect.md b/.changeset/navigate-in-effect.md
new file mode 100644
index 0000000000..bffcb59964
--- /dev/null
+++ b/.changeset/navigate-in-effect.md
@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+Fix detection of `useNavigate` in the render cycle by setting the `activeRef` in a layout effect, allowing the `navigate` function to be passed to child components and called in a `useEffect` there.
diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx
index e0fb652665..eb7a19c438 100644
--- a/packages/react-router/__tests__/useNavigate-test.tsx
+++ b/packages/react-router/__tests__/useNavigate-test.tsx
@@ -167,77 +167,6 @@ describe("useNavigate", () => {
]
`);
});
-
- it("transitions to the new location when called immediately", () => {
- const Home = React.forwardRef(function Home(_props, ref) {
- let navigate = useNavigate();
-
- React.useImperativeHandle(ref, () => ({
- navigate: () => navigate("/about")
- }))
-
- return null
- })
-
- let homeRef;
-
- let renderer: TestRenderer.ReactTestRenderer;
- renderer = TestRenderer.create(
-
-
- homeRef = ref} />} />
- About} />
-
-
- );
-
- TestRenderer.act(() => {
- homeRef.navigate();
- })
-
- expect(renderer.toJSON()).toMatchInlineSnapshot(`
-
- About
-
- `);
- });
-
- it("allows navigation in child useEffects", () => {
- function Child({ onChildRendered }) {
-
- React.useEffect(() => {
- onChildRendered();
- });
-
- return null;
- }
-
- function Parent() {
- let navigate = useNavigate();
-
- let onChildRendered = React.useCallback(() => navigate("/about"), []);
-
- return ;
- }
-
- let renderer: TestRenderer.ReactTestRenderer;
- TestRenderer.act(() => {
- renderer = TestRenderer.create(
-
-
- } />
- About} />
-
-
- );
- });
-
- expect(renderer.toJSON()).toMatchInlineSnapshot(`
-
- About
-
- `);
- });
it("navigates to the new location with empty query string when no query string is provided", () => {
function Home() {
@@ -372,6 +301,208 @@ describe("useNavigate", () => {
);
});
+ describe("navigating in effects versus render", () => {
+ let warnSpy: jest.SpyInstance;
+
+ beforeEach(() => {
+ warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {});
+ });
+
+ afterEach(() => {
+ warnSpy.mockRestore();
+ });
+
+ describe("MemoryRouter", () => {
+ it("does not allow navigation from the render cycle", () => {
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create(
+
+
+ } />
+ About} />
+
+
+ );
+ });
+
+ function Home() {
+ let navigate = useNavigate();
+ navigate("/about");
+ return Home
;
+ }
+
+ // @ts-expect-error
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ Home
+
+ `);
+ expect(warnSpy).toHaveBeenCalledWith(
+ "You should call navigate() in a React.useEffect(), not when your component is first rendered."
+ );
+ });
+
+ it("allows navigation from effects", () => {
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create(
+
+
+ } />
+ About} />
+
+
+ );
+ });
+
+ function Home() {
+ let navigate = useNavigate();
+ React.useEffect(() => navigate("/about"), []);
+ return Home
;
+ }
+
+ // @ts-expect-error
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ About
+
+ `);
+ expect(warnSpy).not.toHaveBeenCalled();
+ });
+
+ it("allows navigation in child useEffects", () => {
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create(
+
+
+ } />
+ About} />
+
+
+ );
+ });
+
+ function Parent() {
+ let navigate = useNavigate();
+ let onChildRendered = React.useCallback(() => navigate("/about"), []);
+ return ;
+ }
+
+ function Child({ onChildRendered }) {
+ React.useEffect(() => onChildRendered());
+ return null;
+ }
+
+ // @ts-expect-error
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ About
+
+ `);
+ });
+ });
+
+ describe("RouterProvider", () => {
+ it("does not allow navigation from the render cycle", async () => {
+ let router = createMemoryRouter([
+ {
+ index: true,
+ Component() {
+ let navigate = useNavigate();
+ navigate("/about");
+ return Home
;
+ },
+ },
+ {
+ path: "about",
+ element: About
,
+ },
+ ]);
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create();
+ });
+
+ // @ts-expect-error
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ Home
+
+ `);
+ expect(warnSpy).toHaveBeenCalledWith(
+ "You should call navigate() in a React.useEffect(), not when your component is first rendered."
+ );
+ });
+
+ it("allows navigation from effects", () => {
+ let router = createMemoryRouter([
+ {
+ index: true,
+ Component() {
+ let navigate = useNavigate();
+ React.useEffect(() => navigate("/about"), []);
+ return Home
;
+ },
+ },
+ {
+ path: "about",
+ element: About
,
+ },
+ ]);
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create();
+ });
+
+ // @ts-expect-error
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ About
+
+ `);
+ expect(warnSpy).not.toHaveBeenCalled();
+ });
+
+ it("allows navigation in child useEffects", () => {
+ let router = createMemoryRouter([
+ {
+ index: true,
+ Component() {
+ let navigate = useNavigate();
+ let onChildRendered = React.useCallback(
+ () => navigate("/about"),
+ []
+ );
+ return ;
+ },
+ },
+ {
+ path: "about",
+ element: About
,
+ },
+ ]);
+ let renderer: TestRenderer.ReactTestRenderer;
+ TestRenderer.act(() => {
+ renderer = TestRenderer.create();
+ });
+
+ function Child({ onChildRendered }) {
+ React.useEffect(() => onChildRendered());
+ return null;
+ }
+
+ // @ts-expect-error
+ expect(renderer.toJSON()).toMatchInlineSnapshot(`
+
+ About
+
+ `);
+ });
+ });
+ });
+
describe("with state", () => {
it("adds the state to location.state", () => {
function Home() {
diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx
index 91e16a16ec..64ad292067 100644
--- a/packages/react-router/lib/hooks.tsx
+++ b/packages/react-router/lib/hooks.tsx
@@ -150,6 +150,21 @@ export interface NavigateFunction {
(delta: number): void;
}
+const navigateEffectWarning =
+ `You should call navigate() in a React.useEffect(), not when ` +
+ `your component is first rendered.`;
+
+// Mute warnings for calls to useNavigate in SSR environments
+function useIsomorphicLayoutEffect(
+ cb: Parameters[0]
+) {
+ let isStatic = React.useContext(NavigationContext).static;
+ if (!isStatic) {
+ // eslint-disable-next-line react-hooks/rules-of-hooks
+ React.useLayoutEffect(cb);
+ }
+}
+
/**
* Returns an imperative method for changing the location. Used by s, but
* may also be used by other elements to change the location.
@@ -180,17 +195,17 @@ function useNavigateUnstable(): NavigateFunction {
);
let activeRef = React.useRef(false);
- React.useLayoutEffect(() => {
+ useIsomorphicLayoutEffect(() => {
activeRef.current = true;
});
let navigate: NavigateFunction = React.useCallback(
(to: To | number, options: NavigateOptions = {}) => {
- warning(
- activeRef.current,
- `You should call navigate() in a React.useEffect(), not when ` +
- `your component is first rendered.`
- );
+ warning(activeRef.current, navigateEffectWarning);
+
+ // Short circuit here since if this happens on first render the navigate
+ // is useless because we haven't wired up our subscriber yet
+ if (!activeRef.current) return;
if (typeof to === "number") {
navigator.go(to);
@@ -929,8 +944,19 @@ function useNavigateStable(): NavigateFunction {
let { router } = useDataRouterContext(DataRouterHook.UseNavigateStable);
let id = useCurrentRouteId(DataRouterStateHook.UseNavigateStable);
+ let activeRef = React.useRef(false);
+ useIsomorphicLayoutEffect(() => {
+ activeRef.current = true;
+ });
+
let navigate: NavigateFunction = React.useCallback(
(to: To | number, options: NavigateOptions = {}) => {
+ warning(activeRef.current, navigateEffectWarning);
+
+ // Short circuit here since if this happens on first render the navigate
+ // is useless because we haven't wired up our subscriber yet
+ if (!activeRef.current) return;
+
if (typeof to === "number") {
router.navigate(to);
} else {
From b116ba470797ae7dd248ab183ce3d4b01c2a9580 Mon Sep 17 00:00:00 2001
From: Matt Brophy
Date: Tue, 25 Apr 2023 10:17:39 -0400
Subject: [PATCH 3/7] Bundle bump
---
package.json | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/package.json b/package.json
index 227697dde6..36d247e07e 100644
--- a/package.json
+++ b/package.json
@@ -108,10 +108,10 @@
"none": "45.8 kB"
},
"packages/react-router/dist/react-router.production.min.js": {
- "none": "12.9 kB"
+ "none": "13.0 kB"
},
"packages/react-router/dist/umd/react-router.production.min.js": {
- "none": "15.3 kB"
+ "none": "15.4 kB"
},
"packages/react-router-dom/dist/react-router-dom.production.min.js": {
"none": "12 kB"
From 22664d52376b45e691429a0ed9fdea0fbba8e846 Mon Sep 17 00:00:00 2001
From: Matt Brophy
Date: Tue, 25 Apr 2023 10:19:10 -0400
Subject: [PATCH 4/7] Fix lint warnings
---
packages/react-router/__tests__/useNavigate-test.tsx | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx
index eb7a19c438..6c601a82c5 100644
--- a/packages/react-router/__tests__/useNavigate-test.tsx
+++ b/packages/react-router/__tests__/useNavigate-test.tsx
@@ -358,7 +358,7 @@ describe("useNavigate", () => {
function Home() {
let navigate = useNavigate();
- React.useEffect(() => navigate("/about"), []);
+ React.useEffect(() => navigate("/about"), [navigate]);
return Home
;
}
@@ -386,7 +386,10 @@ describe("useNavigate", () => {
function Parent() {
let navigate = useNavigate();
- let onChildRendered = React.useCallback(() => navigate("/about"), []);
+ let onChildRendered = React.useCallback(
+ () => navigate("/about"),
+ [navigate]
+ );
return ;
}
@@ -442,7 +445,7 @@ describe("useNavigate", () => {
index: true,
Component() {
let navigate = useNavigate();
- React.useEffect(() => navigate("/about"), []);
+ React.useEffect(() => navigate("/about"), [navigate]);
return Home
;
},
},
@@ -473,7 +476,7 @@ describe("useNavigate", () => {
let navigate = useNavigate();
let onChildRendered = React.useCallback(
() => navigate("/about"),
- []
+ [navigate]
);
return ;
},
From 313fa3352679049ace8a758fa2a692e424291035 Mon Sep 17 00:00:00 2001
From: Matt Brophy
Date: Tue, 25 Apr 2023 10:20:06 -0400
Subject: [PATCH 5/7] Fix comments
---
packages/react-router/lib/hooks.tsx | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx
index 64ad292067..869084d26d 100644
--- a/packages/react-router/lib/hooks.tsx
+++ b/packages/react-router/lib/hooks.tsx
@@ -204,7 +204,7 @@ function useNavigateUnstable(): NavigateFunction {
warning(activeRef.current, navigateEffectWarning);
// Short circuit here since if this happens on first render the navigate
- // is useless because we haven't wired up our subscriber yet
+ // is useless because we haven't wired up our history listener yet
if (!activeRef.current) return;
if (typeof to === "number") {
@@ -954,7 +954,7 @@ function useNavigateStable(): NavigateFunction {
warning(activeRef.current, navigateEffectWarning);
// Short circuit here since if this happens on first render the navigate
- // is useless because we haven't wired up our subscriber yet
+ // is useless because we haven't wired up our router subscriber yet
if (!activeRef.current) return;
if (typeof to === "number") {
From 9fcfc3e672a0f6607e0b14c8834cc60c894eeb17 Mon Sep 17 00:00:00 2001
From: Matt Brophy
Date: Tue, 25 Apr 2023 13:56:43 -0400
Subject: [PATCH 6/7] Add comment with react 18.3 note on useLayoutEffect
---
packages/react-router/lib/hooks.tsx | 2 ++
1 file changed, 2 insertions(+)
diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx
index 869084d26d..be846ef51a 100644
--- a/packages/react-router/lib/hooks.tsx
+++ b/packages/react-router/lib/hooks.tsx
@@ -160,6 +160,8 @@ function useIsomorphicLayoutEffect(
) {
let isStatic = React.useContext(NavigationContext).static;
if (!isStatic) {
+ // We should be able to get rid of this once react 18.3 is released
+ // See: https://github.com/facebook/react/pull/26395
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useLayoutEffect(cb);
}
From 73b00518650f18f2caedc023f1a6f6aa10e8ed09 Mon Sep 17 00:00:00 2001
From: Matt Brophy
Date: Tue, 25 Apr 2023 14:13:44 -0400
Subject: [PATCH 7/7] bump
---
package.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package.json b/package.json
index ee299a214c..f86805f9ae 100644
--- a/package.json
+++ b/package.json
@@ -108,7 +108,7 @@
"none": "45.8 kB"
},
"packages/react-router/dist/react-router.production.min.js": {
- "none": "13 kB"
+ "none": "13.1 kB"
},
"packages/react-router/dist/umd/react-router.production.min.js": {
"none": "15.4 kB"