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 descendant Routes rendering alongside RouterProvider errors #10374

Merged
merged 7 commits into from
Apr 21, 2023
Merged

Fix descendant Routes rendering alongside RouterProvider errors #10374

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fc705d3
Fix descendant Routes rendering alongside RouterProvider errors
brophdawg11 Apr 20, 2023
f1d1e93
Remove it.only
brophdawg11 Apr 20, 2023
90a1851
Remove fork from Routes in favor of new DataRoutes component
brophdawg11 Apr 20, 2023
146237a
Revert whitespace snapshot changes
brophdawg11 Apr 20, 2023
8602a25
bump bundle
brophdawg11 Apr 20, 2023
69e1f2d
Merge branch 'dev' into brophdawg11/routes-with-data-errors
brophdawg11 Apr 21, 2023
11a129a
Merge branch 'dev' into brophdawg11/routes-with-data-errors
brophdawg11 Apr 21, 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/descendant-routes-data-errors.md
View file
Open in desktop
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
55 changes: 50 additions & 5 deletions packages/react-router/__tests__/data-memory-router-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,11 +1019,6 @@
),
{
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 @@
`);
});

it.only("renders <Routes> alongside a data router ErrorBoundary", () => {

Check failure on line 1056 in packages/react-router/__tests__/data-memory-router-test.tsx

View workflow job for this annotation

GitHub Actions / 🧪 Test 

Unexpected focused test
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
12 changes: 9 additions & 3 deletions packages/react-router/lib/hooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,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 @@ -432,7 +433,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 @@ -613,7 +617,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 @@ -635,7 +639,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