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.

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

[Bug]: Navigate malfunction in createBrowserRouter #10417

Closed
abhay187 opened this issue Apr 29, 2023 · 12 comments · Fixed by #10435
Closed

[Bug]: Navigate malfunction in createBrowserRouter #10417

abhay187 opened this issue Apr 29, 2023 · 12 comments · Fixed by #10435
Labels

Comments

@abhay187
Copy link

What version of React Router are you using?

v6.11.0

Steps to Reproduce

Check this repo https://stackblitz.com/edit/react-ts-ffc4fk?file=App.tsx

Expected Behavior

When I go to dashboard route it should route to /dashboard/profile

Actual Behavior

it is routing to /dashboard/profile/profile

@abhay187 abhay187 added the bug label Apr 29, 2023
@abhay187
Copy link
Author

The code was working as expected in v6.10.0

@huijiewei
Copy link

same bug, only in the development env

@virendave96
Copy link

My code earlier.

 <Routes>
  <Route index element={<Navigate to="db" />} />
  <Route path="db" element={<Db/>} />
  </Routes>

Working earlier but now I have to

<Routes>
  <Route index element={<Navigate to="/app/db" />} />
  <Route path="db" element={<Db/>} />
</Routes>

@abhay187
Copy link
Author

abhay187 commented May 1, 2023

@huijiewei Yes this issue is in dev env and upon inspection I realize it is occurring due to StrictMode.

@brophdawg11
Copy link
Contributor

Yeah this is definitely due to StrictMode firing effects twice and the inability to "cleanup" from a navigation. It seems the reason this worked in 6.10 was the use of useSyncExternalStore inside React Router which flushed certain updates synchronously such that things were reflected in the second StrictMode effect.

The usage of useSyncExternalStore caused some super nuanced react-state-syncing bugs so we've replaced it with useState in 6.11, and now the state is not flushed synchronously.

I would argue that this is sort of "working as React expects" in that when you use <Navigate> you case a navigation side effect and there is no "cleanup" - so it happens twice.

The idiomatic way to do this with RouterProvider is to decouple navigation from rendering, and instead redirect from a loader instead of from the render cycle:

{
  index: true,
  loader: () => redirect("profile"),
},

I'll dig in a bit deeper but I'm not sure if there's much we can do about this unfortunately since we don't have visibility into the pending async state in the second navigate 😕 .

@reintroducing
Copy link

This is actually also breaking any call to Navigate in my app as well. This works fine in 6.10.0 but completely breaks in 6.11.0. The problem is compounded with a dynamic portion in the path. Take the following code:

<Route
    path=":username"
    element={<Suspend child={<PatientContainer />} />}
    errorElement={<ErrorDisplayBoundary />}
>
    <Route index element={<Navigate to="overview" replace />} />
    <Route
        path="overview"
        element={<Suspend child={<Overview />} />}
        errorElement={<ErrorDisplayBoundary />}
    />
</Route>

In 6.10.0, if you landed on patient/:username (this is a nested route) you were redirected to patient/:username/overview, but in 6.11.0 this redirects to patient/overview, thus completely removing the dynamic username out of the URL and completely breaking the page (because the username is used in API loading for elements on that page).

Same goes for anywhere else that I have a Navigate component.

return createBrowserRouter([
    {
        path: '/',
        element: <Index />,
        errorElement: <ErrorDisplayBoundary />,
        children: [
            {
                index: true,
                element: <Navigate to="/reporting" replace />,
            },
            {
                path: 'census/*',
                element: <Navigate to="/members" replace />,
            },

These two routes redirected correctly, from reporting to reporting/overview (because the base of reporting redirects to reporting/overview) and same for members to members/active (since that is the redirect that should happen), but now they redirect to overview and active respectively, which are not proper routes.

@brophdawg11
Copy link
Contributor

@reintroducing Is there a specific reason you're using <Navigate> and not redirecting from a loader in your RouterProvider application? Are the <Navigate> usages from a migration of a BrowserRouter app? Or was that approach written net-new?

@reintroducing
Copy link

@brophdawg11 Yes, they were migration from BrowserRouter. We don't use loader in our app because we have nested routes defined at different levels of the app which was my understanding that you cannot do that when using loaders (the whole route tree has to be defined in one file at the root level to take advantage of that). Is this incorrect? And is my usage of Navigate incorrect? Is there a different way to do this that I'm not aware of and we shouldn't be using Navigate?

@brophdawg11
Copy link
Contributor

Thanks for clarifying @reintroducing.

In a mixed app - you can use loaders on any routes defined via createBrowserRouter - so specifically <Navigate to="/reporting" replace /> and <Navigate to="/members" replace /> should be able to be redirects from a loader.

But you are correct that any <Route>'s defined in a descendant <Routes> cannot have loaders and you would need to use <Navigate>. I think your specific issue might also be caused by #10430.

We should be getting a prerelease out soon with these fixes that you can test out 👍

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

🤖 Hello there,

We just published version 6.11.1-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot closed this as completed May 2, 2023
@reintroducing
Copy link

@brophdawg11 i just tested 6.11.1-pre.0 and i can verify that it fixes all the issues I was seeing. thank you!

@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

🤖 Hello there,

We just published version 6.11.1 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants