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 usage of Navigate in strict mode when using a data router #10435

Merged
merged 4 commits into from
May 2, 2023
Merged

Fix usage of Navigate in strict mode when using a data router #10435

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19204a1
Fix usage of Navigate in strict mode when using a data router
brophdawg11 May 2, 2023
d942ed6
Bump bundle
brophdawg11 May 2, 2023
93ccb2b
Add tests for different setState-driven Navigte renders
brophdawg11 May 2, 2023
7f83a20
update test
brophdawg11 May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/navigate-strict-mode.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router": patch
---

Fix usage of `<Navigate>` in strict mode when using a data router
82 changes: 82 additions & 0 deletions packages/react-router/__tests__/navigate-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,88 @@ describe("<Navigate>", () => {
</div>"
`);
});

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: () => <Navigate to={"b"} replace />,
},
{
path: "b",
element: <h1>Page B</h1>,
},
],
},
],
{ initialEntries: ["/a"] }
);

let { container } = render(
<React.StrictMode>
<RouterProvider router={router} />
</React.StrictMode>
);

await waitFor(() => screen.getByText("Page B"));

expect(getHtml(container)).toMatchInlineSnapshot(`
"<div>
<h1>
Page B
</h1>
</div>"
`);
});

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: () => <Navigate to={"b"} replace />,
},
{
path: "b",
async loader() {
await new Promise((r) => setTimeout(r, 10));
return null;
},
element: <h1>Page B</h1>,
},
],
},
],
{ initialEntries: ["/a"] }
);

let { container } = render(
<React.StrictMode>
<RouterProvider router={router} />
</React.StrictMode>
);

await waitFor(() => screen.getByText("Page B"));

expect(getHtml(container)).toMatchInlineSnapshot(`
"<div>
<h1>
Page B
</h1>
</div>"
`);
});
});

function getHtml(container: HTMLElement) {
Expand Down
30 changes: 20 additions & 10 deletions packages/react-router/lib/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -34,6 +36,7 @@ import {
DataRouterContext,
DataRouterStateContext,
AwaitContext,
RouteContext,
} from "./context";
import {
useAsyncValue,
Expand All @@ -43,6 +46,7 @@ import {
useRoutes,
_renderMatches,
useRoutesImpl,
useLocation,
} from "./hooks";

export interface RouterProviderProps {
Expand Down Expand Up @@ -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;
}
Comment on lines -224 to -226
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer need this now that the effect has a proper dependency array - since it won't re-fire on the second render because the path own't have changed.

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]
);
Comment on lines +235 to +238
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason this was problematic in 6.11 is that we would call navigate twice and when in a RouterProvider we'd delay resolving the relative path until inside the router. And thus the second execution would re-resolve to against the current location (which in a on-loader scenario would already be updated from the first effect).

Instead, we resolve the absolute path here in Navigate so that duplicate calls to navigate via the data router go to the same path - just as they do in BrowserRouter.

Copy link
Member

@jacob-ebey jacob-ebey May 2, 2023 •
edited

Choose a reason for hiding this comment

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

I'm a little worried about state being in there. I'd still like to see this be an empty array as I think we are going to see bad app/library provider code that immediately flows new state / props down causing a new state identity due to no-one ever in the history of app code memoing the value they pass in. If tests are passing across react v17 and 18 though I'm fine shipping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For future reference we tested the unit test setups from 93ccb2b in demo apps using react 16.8 and 18 and confirmed the UI was the same (even if the underlying React.StrictMode execution approaches differed). We may try to see if we can get our test suite running against both versions in a separate undertaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think we're no worse off than we were previously where the effect had no second param since it would have re-run every time anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link

Choose a reason for hiding this comment

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

I'm a little worried about state being in there. I'd still like to see this be an empty array as I think we are going to see bad app/library provider code that immediately flows new state / props down causing a new state identity due to no-one ever in the history of app code memoing the value they pass in. If tests are passing across react v17 and 18 though I'm fine shipping it.

This wound up biting me. I had an object that I was passing to state that wasn't memo'd. I think the check for dataRouterState !== "idle" saved me prior to 6.11.1, but with this change I had a flurry of redirects (each calling a loader and reloading a file hundreds of times).


return null;
}
Expand Down