From 19204a11cdcd7ad623a3ee11c95948eadc69f672 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 2 May 2023 14:06:39 -0400 Subject: [PATCH 1/4] Fix usage of Navigate in strict mode when using a data router --- .changeset/navigate-strict-mode.md | 5 ++ .../react-router/__tests__/navigate-test.tsx | 82 +++++++++++++++++++ packages/react-router/lib/components.tsx | 30 ++++--- 3 files changed, 107 insertions(+), 10 deletions(-) create mode 100644 .changeset/navigate-strict-mode.md diff --git a/.changeset/navigate-strict-mode.md b/.changeset/navigate-strict-mode.md new file mode 100644 index 0000000000..e417ed415b --- /dev/null +++ b/.changeset/navigate-strict-mode.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Fix usage of `` in strict mode when using a data router diff --git a/packages/react-router/__tests__/navigate-test.tsx b/packages/react-router/__tests__/navigate-test.tsx index f0b9e71644..22f97c3d5d 100644 --- a/packages/react-router/__tests__/navigate-test.tsx +++ b/packages/react-router/__tests__/navigate-test.tsx @@ -485,6 +485,88 @@ describe("", () => { " `); }); + + it("handles sync relative navigations in StrictMode using a data router", async () => { + const router = createMemoryRouter( + [ + { + path: "/a", + children: [ + { + index: true, + // This is a relative navigation from the current location of /a. + // Ensure we don't route from /a -> /a/b -> /a/b/b + Component: () => , + }, + { + path: "b", + element:

Page B

, + }, + ], + }, + ], + { initialEntries: ["/a"] } + ); + + let { container } = render( + + + + ); + + await waitFor(() => screen.getByText("Page B")); + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+

+ Page B +

+
" + `); + }); + + it("handles async relative navigations in StrictMode using a data router", async () => { + const router = createMemoryRouter( + [ + { + path: "/a", + children: [ + { + index: true, + // This is a relative navigation from the current location of /a. + // Ensure we don't route from /a -> /a/b -> /a/b/b + Component: () => , + }, + { + path: "b", + async loader() { + await new Promise((r) => setTimeout(r, 10)); + return null; + }, + element:

Page B

, + }, + ], + }, + ], + { initialEntries: ["/a"] } + ); + + let { container } = render( + + + + ); + + await waitFor(() => screen.getByText("Page B")); + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+

+ Page B +

+
" + `); + }); }); function getHtml(container: HTMLElement) { diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 1c526b830d..56ef4c1507 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -16,8 +16,10 @@ import { createMemoryHistory, UNSAFE_invariant as invariant, parsePath, + resolveTo, stripBasename, UNSAFE_warning as warning, + UNSAFE_getPathContributingMatches as getPathContributingMatches, } from "@remix-run/router"; import type { @@ -34,6 +36,7 @@ import { DataRouterContext, DataRouterStateContext, AwaitContext, + RouteContext, } from "./context"; import { useAsyncValue, @@ -43,6 +46,7 @@ import { useRoutes, _renderMatches, useRoutesImpl, + useLocation, } from "./hooks"; export interface RouterProviderProps { @@ -214,18 +218,24 @@ export function Navigate({ `only ever rendered in response to some user interaction or state change.` ); - let dataRouterState = React.useContext(DataRouterStateContext); + let { matches } = React.useContext(RouteContext); + let { pathname: locationPathname } = useLocation(); let navigate = useNavigate(); - React.useEffect(() => { - // Avoid kicking off multiple navigations if we're in the middle of a - // data-router navigation, since components get re-rendered when we enter - // a submitting/loading state - if (dataRouterState && dataRouterState.navigation.state !== "idle") { - return; - } - navigate(to, { replace, state, relative }); - }); + // Resolve the path outside of the effect so that when effects run twice in + // StrictMode they navigate to the same place + let path = resolveTo( + to, + getPathContributingMatches(matches).map((match) => match.pathnameBase), + locationPathname, + relative === "path" + ); + let jsonPath = JSON.stringify(path); + + React.useEffect( + () => navigate(JSON.parse(jsonPath), { replace, state, relative }), + [navigate, jsonPath, relative, replace, state] + ); return null; } From d942ed6250b78f271344886d83bb1d102e2cc3e5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 2 May 2023 14:31:43 -0400 Subject: [PATCH 2/4] Bump bundle --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 6bc8bb70bc..8b95a2d15f 100644 --- a/package.json +++ b/package.json @@ -108,10 +108,10 @@ "none": "45 kB" }, "packages/react-router/dist/react-router.production.min.js": { - "none": "13.1 kB" + "none": "13.3 kB" }, "packages/react-router/dist/umd/react-router.production.min.js": { - "none": "15.4 kB" + "none": "15.6 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { "none": "11.8 kB" From 93ccb2b5d967142270a084cdd166bcc34943fefb Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 2 May 2023 16:23:22 -0400 Subject: [PATCH 3/4] Add tests for different setState-driven Navigte renders --- .../react-router/__tests__/navigate-test.tsx | 286 ++++++++++++++++++ 1 file changed, 286 insertions(+) diff --git a/packages/react-router/__tests__/navigate-test.tsx b/packages/react-router/__tests__/navigate-test.tsx index 22f97c3d5d..629cf16c8c 100644 --- a/packages/react-router/__tests__/navigate-test.tsx +++ b/packages/react-router/__tests__/navigate-test.tsx @@ -567,6 +567,292 @@ describe("", () => { " `); }); + + it("handles setState in render in StrictMode using a data router (sync loader)", async () => { + let renders: number[] = []; + const router = createMemoryRouter([ + { + path: "/", + children: [ + { + index: true, + Component() { + let [count, setCount] = React.useState(0); + if (count === 0) { + setCount(1); + } + return ; + }, + }, + { + path: "b", + Component() { + let { state } = useLocation() as { state: { count: number } }; + renders.push(state.count); + return ( + <> +

Page B

+

{state.count}

+ + ); + }, + }, + ], + }, + ]); + + let navigateSpy = jest.spyOn(router, "navigate"); + + let { container } = render( + + + + ); + + await waitFor(() => screen.getByText("Page B")); + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+

+ Page B +

+

+ 1 +

+
" + `); + expect(navigateSpy).toHaveBeenCalledTimes(2); + expect(navigateSpy.mock.calls[0]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 1 } }, + ]); + expect(navigateSpy.mock.calls[1]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 1 } }, + ]); + expect(renders).toEqual([1, 1]); + }); + + it("handles setState in effect in StrictMode using a data router (sync loader)", async () => { + let renders: number[] = []; + const router = createMemoryRouter([ + { + path: "/", + children: [ + { + index: true, + Component() { + // When state managed by react and changes during render, we'll + // only "see" the value from the first pass through here in our + // effects + let [count, setCount] = React.useState(0); + React.useEffect(() => { + if (count === 0) { + setCount(1); + } + }, [count]); + return ; + }, + }, + { + path: "b", + Component() { + let { state } = useLocation() as { state: { count: number } }; + renders.push(state.count); + return ( + <> +

Page B

+

{state.count}

+ + ); + }, + }, + ], + }, + ]); + + let navigateSpy = jest.spyOn(router, "navigate"); + + let { container } = render( + + + + ); + + await waitFor(() => screen.getByText("Page B")); + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+

+ Page B +

+

+ 0 +

+
" + `); + expect(navigateSpy).toHaveBeenCalledTimes(2); + expect(navigateSpy.mock.calls[0]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 0 } }, + ]); + expect(navigateSpy.mock.calls[1]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 0 } }, + ]); + expect(renders).toEqual([0, 0]); + }); + + it("handles setState in render in StrictMode using a data router (async loader)", async () => { + let renders: number[] = []; + const router = createMemoryRouter([ + { + path: "/", + children: [ + { + index: true, + Component() { + let [count, setCount] = React.useState(0); + if (count === 0) { + setCount(1); + } + return ; + }, + }, + { + path: "b", + async loader() { + await new Promise((r) => setTimeout(r, 10)); + return null; + }, + Component() { + let { state } = useLocation() as { state: { count: number } }; + renders.push(state.count); + return ( + <> +

Page B

+

{state.count}

+ + ); + }, + }, + ], + }, + ]); + + let navigateSpy = jest.spyOn(router, "navigate"); + + let { container } = render( + + + + ); + + await waitFor(() => screen.getByText("Page B")); + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+

+ Page B +

+

+ 1 +

+
" + `); + expect(navigateSpy).toHaveBeenCalledTimes(2); + expect(navigateSpy.mock.calls[0]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 1 } }, + ]); + expect(navigateSpy.mock.calls[1]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 1 } }, + ]); + // /a/b rendered with the same state value both times + expect(renders).toEqual([1, 1]); + }); + + it("handles setState in effect in StrictMode using a data router (async loader)", async () => { + let renders: number[] = []; + const router = createMemoryRouter([ + { + path: "/", + children: [ + { + index: true, + Component() { + // When state managed by react and changes during render, we'll + // only "see" the value from the first pass through here in our + // effects + let [count, setCount] = React.useState(0); + React.useEffect(() => { + if (count === 0) { + setCount(1); + } + }, [count]); + return ; + }, + }, + { + path: "b", + async loader() { + await new Promise((r) => setTimeout(r, 10)); + return null; + }, + Component() { + let { state } = useLocation() as { state: { count: number } }; + renders.push(state.count); + return ( + <> +

Page B

+

{state.count}

+ + ); + }, + }, + ], + }, + ]); + + let navigateSpy = jest.spyOn(router, "navigate"); + + let { container } = render( + + + + ); + + await waitFor(() => screen.getByText("Page B")); + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+

+ Page B +

+

+ 1 +

+
" + `); + expect(navigateSpy).toHaveBeenCalledTimes(3); + expect(navigateSpy.mock.calls[0]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 0 } }, + ]); + expect(navigateSpy.mock.calls[1]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 0 } }, + ]); + // StrictMode only applies the double-effect execution on component mount, + // not component update + expect(navigateSpy.mock.calls[2]).toMatchObject([ + { pathname: "/b" }, + { state: { count: 1 } }, + ]); + // /a/b rendered with the latest state value both times + expect(renders).toEqual([1, 1]); + }); }); function getHtml(container: HTMLElement) { From 7f83a20b7847bc14a5d7a65c58971229e5348a66 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 2 May 2023 16:30:04 -0400 Subject: [PATCH 4/4] update test --- .../react-router/__tests__/navigate-test.tsx | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/packages/react-router/__tests__/navigate-test.tsx b/packages/react-router/__tests__/navigate-test.tsx index 629cf16c8c..5342758b5e 100644 --- a/packages/react-router/__tests__/navigate-test.tsx +++ b/packages/react-router/__tests__/navigate-test.tsx @@ -487,26 +487,23 @@ describe("", () => { }); it("handles sync relative navigations in StrictMode using a data router", async () => { - const router = createMemoryRouter( - [ - { - path: "/a", - children: [ - { - index: true, - // This is a relative navigation from the current location of /a. - // Ensure we don't route from /a -> /a/b -> /a/b/b - Component: () => , - }, - { - path: "b", - element:

Page B

, - }, - ], - }, - ], - { initialEntries: ["/a"] } - ); + const router = createMemoryRouter([ + { + path: "/", + children: [ + { + index: true, + // This is a relative navigation from the current location of /a. + // Ensure we don't route from / -> /b -> /b/b + Component: () => , + }, + { + path: "b", + element:

Page B

, + }, + ], + }, + ]); let { container } = render(