-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Changes from all commits
fc705d3
f1d1e93
90a1851
146237a
8602a25
69e1f2d
11a129a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"react-router": patch | ||
--- | ||
|
||
Fix bug preventing rendering of descendant `<Routes>` when `RouterProvider` errors existed |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1019,11 +1019,6 @@ describe("createMemoryRouter", () => { | |
), | ||
{ | ||
initialEntries: ["/deep/path/to/descendant/routes"], | ||
hydrationData: { | ||
loaderData: { | ||
"0-0": "count=1", | ||
}, | ||
}, | ||
} | ||
); | ||
let { container } = render(<RouterProvider router={router} />); | ||
|
@@ -1058,6 +1053,56 @@ describe("createMemoryRouter", () => { | |
`); | ||
}); | ||
|
||
it("renders <Routes> alongside a data router ErrorBoundary", () => { | ||
let router = createMemoryRouter( | ||
[ | ||
{ | ||
path: "*", | ||
Component() { | ||
return ( | ||
<> | ||
<Outlet /> | ||
<Routes> | ||
<Route index element={<h1>Descendant</h1>} /> | ||
</Routes> | ||
Comment on lines
+1064
to
+1067
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
</> | ||
); | ||
}, | ||
children: [ | ||
{ | ||
id: "index", | ||
index: true, | ||
Component: () => <h1>Child</h1>, | ||
ErrorBoundary() { | ||
return <p>{(useRouteError() as Error).message}</p>; | ||
}, | ||
}, | ||
], | ||
}, | ||
], | ||
{ | ||
initialEntries: ["/"], | ||
hydrationData: { | ||
errors: { | ||
index: new Error("Broken!"), | ||
}, | ||
}, | ||
} | ||
); | ||
let { container } = render(<RouterProvider router={router} />); | ||
|
||
expect(getHtml(container)).toMatchInlineSnapshot(` | ||
"<div> | ||
<p> | ||
Broken! | ||
</p> | ||
<h1> | ||
Descendant | ||
</h1> | ||
</div>" | ||
`); | ||
}); | ||
|
||
describe("errors", () => { | ||
it("renders hydration errors on leaf elements using errorElement", async () => { | ||
let router = createMemoryRouter( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,7 +116,11 @@ export function RouterProvider({ | |
navigationType={router.state.historyAction} | ||
navigator={navigator} | ||
> | ||
{router.state.initialized ? <Routes /> : fallbackElement} | ||
{router.state.initialized ? ( | ||
<DataRoutes routes={router.routes} /> | ||
) : ( | ||
fallbackElement | ||
)} | ||
</Router> | ||
</DataRouterStateContext.Provider> | ||
</DataRouterContext.Provider> | ||
|
@@ -125,6 +129,14 @@ export function RouterProvider({ | |
); | ||
} | ||
|
||
function DataRoutes({ | ||
routes, | ||
}: { | ||
routes: DataRouteObject[]; | ||
}): React.ReactElement | null { | ||
return useRoutes(routes); | ||
} | ||
|
||
export interface MemoryRouterProps { | ||
basename?: string; | ||
children?: React.ReactNode; | ||
|
@@ -393,15 +405,7 @@ export function Routes({ | |
children, | ||
location, | ||
}: RoutesProps): React.ReactElement | null { | ||
let dataRouterContext = React.useContext(DataRouterContext); | ||
// When in a DataRouterContext _without_ children, we use the router routes | ||
// directly. If we have children, then we're in a descendant tree and we | ||
// need to use child routes. | ||
let routes = | ||
dataRouterContext && !children | ||
? (dataRouterContext.router.routes as DataRouteObject[]) | ||
: createRoutesFromChildren(children); | ||
return useRoutes(routes, location); | ||
return useRoutes(createRoutesFromChildren(children), location); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now back to only being used for non-data routers |
||
} | ||
|
||
export interface AwaitResolveRenderFunction { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -320,6 +320,7 @@ export function useRoutes( | |
); | ||
|
||
let { navigator } = React.useContext(NavigationContext); | ||
let dataRouterContext = React.useContext(DataRouterContext); | ||
let dataRouterStateContext = React.useContext(DataRouterStateContext); | ||
let { matches: parentMatches } = React.useContext(RouteContext); | ||
let routeMatch = parentMatches[parentMatches.length - 1]; | ||
|
@@ -433,7 +434,10 @@ export function useRoutes( | |
}) | ||
), | ||
parentMatches, | ||
dataRouterStateContext || undefined | ||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the root of the bug - export function useRoutes(
routes: RouteObject[],
locationArg?: Partial<Location> | string,
isDataRouterRender?: boolean, // New arg passed true from <DataRoutes>
) |
||
); | ||
|
||
// When a user passes in a `locationArg`, the associated routes need to | ||
|
@@ -622,7 +626,7 @@ function RenderedRoute({ routeContext, match, children }: RenderedRouteProps) { | |
export function _renderMatches( | ||
matches: RouteMatch[] | null, | ||
parentMatches: RouteMatch[] = [], | ||
dataRouterState?: RemixRouter["state"] | ||
dataRouterState: RemixRouter["state"] | null = null | ||
): React.ReactElement | null { | ||
if (matches == null) { | ||
if (dataRouterState?.errors) { | ||
|
@@ -644,7 +648,9 @@ export function _renderMatches( | |
); | ||
invariant( | ||
errorIndex >= 0, | ||
`Could not find a matching route for the current errors: ${errors}` | ||
`Could not find a matching route for errors on route IDs: ${Object.keys( | ||
errors | ||
).join(",")}` | ||
); | ||
renderedMatches = renderedMatches.slice( | ||
0, | ||
|
There was a problem hiding this comment.
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 touseRoutes
this way instead of reaching back into context