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

Fix descendant Routes rendering alongside RouterProvider errors #10374

Merged
merged 7 commits into from Apr 21, 2023

Conversation

brophdawg11
Copy link
Contributor

Fix descendant <Routes> rendering inside a RouterProvider when data routes errors exist

Closes #10317

@changeset-bot
Copy link

changeset-bot bot commented Apr 20, 2023

🦋 Changeset detected

Latest commit: 11a129a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 linked an issue Apr 20, 2023 that may be closed by this pull request
Comment on lines +1064 to +1067
<Outlet />
<Routes>
<Route index element={<h1>Descendant</h1>} />
</Routes>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of an unusual use-case. Normally the presence of an error would mean we don't reach descendant <Routes> down inside children Component's. But if the <Routes> is next to the <Outlet /> then both can render at the same time.

routes: DataRouteObject[];
}): React.ReactElement | null {
return useRoutes(routes);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows us to remove the fork from <Routes> since we can pass the routes more directly to useRoutes this way instead of reaching back into context

? (dataRouterContext.router.routes as DataRouteObject[])
: createRoutesFromChildren(children);
return useRoutes(routes, location);
return useRoutes(createRoutesFromChildren(children), location);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now back to only being used for non-data routers

// Only pass along the dataRouterStateContext when we're rendering from the
// RouterProvider layer. If routes is different then we're rendering from
// a descendant <Routes> tree
dataRouterContext?.router.routes === routes ? dataRouterStateContext : null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the root of the bug - _renderMatches was behaving as if in a data router anywhere under the context, including in descendant <Routes>. A potentially more explicit fix would be an optional prop to useRoutes but I was on the fence about touching the public API:

export function useRoutes(
  routes: RouteObject[],
  locationArg?: Partial<Location> | string,
  isDataRouterRender?: boolean,  // New arg passed true from <DataRoutes>
)

@jasikpark
Copy link
Contributor

excited to test this fix! 😄

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.0-pre.0 which includes this pull request. 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
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.0 which includes this pull request. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Nested routing (<Routes> inside <RouterProvider />)
4 participants