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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/descendant-routes-data-errors.md
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
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,10 @@
"none": "45.8 kB"
},
"packages/react-router/dist/react-router.production.min.js": {
"none": "13.5 kB"
"none": "13.6 kB"
},
"packages/react-router/dist/umd/react-router.production.min.js": {
"none": "15.8 kB"
"none": "15.9 kB"
},
"packages/react-router-dom/dist/react-router-dom.production.min.js": {
"none": "12 kB"
Expand Down
19 changes: 16 additions & 3 deletions packages/react-router-dom/server.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@ import {
UNSAFE_convertRoutesToDataRoutes as convertRoutesToDataRoutes,
} from "@remix-run/router";
import { UNSAFE_mapRouteProperties as mapRouteProperties } from "react-router";
import type { Location, RouteObject, To } from "react-router-dom";
import { Routes } from "react-router-dom";
import type {
DataRouteObject,
Location,
RouteObject,
To,
} from "react-router-dom";
import {
createPath,
parsePath,
Router,
useRoutes,
UNSAFE_DataRouterContext as DataRouterContext,
UNSAFE_DataRouterStateContext as DataRouterStateContext,
} from "react-router-dom";
Expand Down Expand Up @@ -127,7 +132,7 @@ export function StaticRouterProvider({
navigationType={dataRouterContext.router.state.historyAction}
navigator={dataRouterContext.navigator}
>
<Routes />
<DataRoutes routes={router.routes} />
</Router>
</DataRouterStateContext.Provider>
</DataRouterContext.Provider>
Expand All @@ -142,6 +147,14 @@ export function StaticRouterProvider({
);
}

function DataRoutes({
routes,
}: {
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


function serializeErrors(
errors: StaticHandlerContext["errors"]
): StaticHandlerContext["errors"] {
Expand Down
55 changes: 50 additions & 5 deletions packages/react-router/__tests__/data-memory-router-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1019,11 +1019,6 @@ describe("createMemoryRouter", () => {
),
{
initialEntries: ["/deep/path/to/descendant/routes"],
hydrationData: {
loaderData: {
"0-0": "count=1",
},
},
}
);
let { container } = render(<RouterProvider router={router} />);
Expand Down Expand Up @@ -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
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.

</>
);
},
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(
Expand Down
24 changes: 14 additions & 10 deletions packages/react-router/lib/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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;
Expand Down Expand Up @@ -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);
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

}

export interface AwaitResolveRenderFunction {
Expand Down
12 changes: 9 additions & 3 deletions packages/react-router/lib/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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
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>
)

);

// When a user passes in a `locationArg`, the associated routes need to
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down