Skip to content

Commit

Permalink
Allow nested absolute paths
Browse files Browse the repository at this point in the history
- Nested routes with absolute paths that do not match their parent
  path are now an error
- Also adds <Route index> prop for index routes

Fixes #7335
  • Loading branch information
mjackson authored and chaance committed Sep 2, 2021
1 parent bf679c4 commit 23a19c9
Show file tree
Hide file tree
Showing 8 changed files with 137 additions and 41 deletions.
1 change: 0 additions & 1 deletion packages/react-router-dom/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ export type {
Navigator,
OutletProps,
Params,
PartialRouteObject,
PathMatch,
RouteMatch,
RouteObject,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
describe("absolute paths", () => {
it.todo("matches when the path matches");
it.todo("throws when the nested path does not begin with its parent path");
});
3 changes: 3 additions & 0 deletions packages/react-router/__tests__/index-routes-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
describe("index routes", () => {
it.todo("throws when the index route has children");
});
10 changes: 5 additions & 5 deletions packages/react-router/__tests__/path-matching-test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { matchRoutes } from "react-router";
import type { PartialRouteObject } from "react-router";
import { matchRoutes } from "react-router";

describe("path matching", () => {
function pickPaths(routes: PartialRouteObject[], pathname: string) {
Expand Down Expand Up @@ -71,7 +71,7 @@ describe("path matching", () => {
children: [{ path: "subjects" }]
},
{ path: "new" },
{ path: "/" },
{ index: true },
{ path: "*" }
]
},
Expand All @@ -87,7 +87,7 @@ describe("path matching", () => {
{ path: "*" }
];

expect(pickPaths(routes, "/courses")).toEqual(["courses", "/"]);
expect(pickPaths(routes, "/courses")).toEqual(["courses", ""]);
expect(pickPaths(routes, "/courses/routing")).toEqual(["courses", ":id"]);
expect(pickPaths(routes, "/courses/routing/subjects")).toEqual([
"courses",
Expand Down Expand Up @@ -115,7 +115,7 @@ describe("path matching", () => {
let routes = [
{
path: ":page",
children: [{ path: "/" }]
children: [{ index: true }]
},
{ path: "page" }
];
Expand All @@ -125,7 +125,7 @@ describe("path matching", () => {
});

describe("path matching with a basename", () => {
let routes = [
let routes: PartialRouteObject[] = [
{
path: "/users/:userId",
children: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,57 @@ import { act, create as createTestRenderer } from "react-test-renderer";
import { MemoryRouter as Router, Outlet, Routes, Route } from "react-router";
import type { ReactTestRenderer } from "react-test-renderer";

describe("nested routes with no path", () => {
it("matches them depth-first", () => {
let renderer!: ReactTestRenderer;
act(() => {
renderer = createTestRenderer(
<Router initialEntries={["/"]}>
<Routes>
<Route element={<First />}>
<Route element={<Second />}>
<Route element={<Third />} />
</Route>
</Route>
</Routes>
</Router>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<div>
First
<div>
Second
<div>
Third
</div>
</div>
</div>
`);
});

function First() {
return (
<div>
First <Outlet />
</div>
);
}

function Second() {
return (
<div>
Second <Outlet />
</div>
);
}

function Third() {
return <div>Third</div>;
}
});

describe("nested /", () => {
it("matches them depth-first", () => {
let renderer!: ReactTestRenderer;
Expand Down
9 changes: 5 additions & 4 deletions packages/react-router/__tests__/route-matching-test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as React from "react";
import { create as createTestRenderer } from "react-test-renderer";
import type { PartialRouteObject } from "react-router";
import {
MemoryRouter as Router,
Outlet,
Expand All @@ -8,7 +9,6 @@ import {
useParams,
useRoutes
} from "react-router";
import type { PartialRouteObject } from "react-router";
import type { InitialEntry } from "history";

describe("route matching", () => {
Expand All @@ -28,7 +28,7 @@ describe("route matching", () => {
children: [{ path: "grades", element: <CourseGrades /> }]
},
{ path: "new", element: <NewCourse /> },
{ path: "/", element: <CoursesIndex /> },
{ index: true, element: <CoursesIndex /> },
{ path: "*", element: <CoursesNotFound /> }
]
},
Expand Down Expand Up @@ -56,7 +56,7 @@ describe("route matching", () => {
<Route path="grades" element={<CourseGrades />} />
</Route>
<Route path="new" element={<NewCourse />} />
<Route path="/" element={<CoursesIndex />} />
<Route index element={<CoursesIndex />} />
<Route path="*" element={<CoursesNotFound />} />
</Route>
<Route path="courses" element={<Landing />}>
Expand All @@ -80,7 +80,7 @@ describe("route matching", () => {
<CourseGrades path="grades" />
</Course>
<NewCourse path="new" />
<CoursesIndex path="/" />
<CoursesIndex index />
<CoursesNotFound path="*" />
</Courses>
<Landing path="courses">
Expand Down Expand Up @@ -193,4 +193,5 @@ describe("route matching", () => {
interface Props {
children?: React.ReactNode;
path?: string;
index?: boolean;
}
4 changes: 2 additions & 2 deletions packages/react-router/__tests__/useRoutes-test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as React from "react";
import * as ReactDOM from "react-dom";
import { create as createTestRenderer } from "react-test-renderer";
import type { RouteObject } from "react-router";
import { MemoryRouter as Router, useRoutes } from "react-router";
import { act } from "react-dom/test-utils";
import type { PartialRouteObject } from "react-router";

describe("useRoutes", () => {
it("returns the matching element from a route config", () => {
Expand Down Expand Up @@ -59,7 +59,7 @@ function RoutesRenderer({
basename,
location
}: {
routes: PartialRouteObject[];
routes: Partial<RouteObject>[];
basename?: string;
location?: Partial<Location> & { pathname: string };
}) {
Expand Down
96 changes: 67 additions & 29 deletions packages/react-router/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ export interface RouteProps {
caseSensitive?: boolean;
children?: React.ReactNode;
element?: React.ReactElement | null;
index?: boolean;
path?: string;
}

Expand Down Expand Up @@ -528,13 +529,34 @@ function useRoutes_(
// You won't get a warning about 2 different <Routes> under a <Route>
// without a trailing *, but this is a best-effort warning anyway since we
// cannot even give the warning unless they land at the parent route.
//
// Example:
//
// <Routes>
// {/* This route path MUST end with /* because otherwise
// it will never match /blog/post/123 */}
// <Route path="blog" element={<Blog />} />
// <Route path="blog/feed" element={<BlogFeed />} />
// </Routes>
//
// function Blog() {
// return (
// <>
// <Link to="post/123">123</Link>
// <Routes>
// <Route path="post/:id" element={<Post />} />
// </Routes>
// </>
// )
// }
// function Post() { ... }
let parentPath = parentRoute && parentRoute.path;
warningOnce(
parentPathname,
!parentRoute || parentRoute.path.endsWith("*"),
`You rendered descendant <Routes> (or called \`useRoutes\`) at ` +
`"${parentPathname}" (under <Route path="${parentPath}">) but the ` +
`parent route path has no trailing "*". This means if you navigate ` +
`parent route path has no trailing "/*". This means if you navigate ` +
`deeper, the parent won't match anymore and therefore the child ` +
`routes will never render.\n\n` +
`Please change the parent <Route path="${parentPath}"> to <Route ` +
Expand All @@ -550,12 +572,14 @@ function useRoutes_(
let location = locationOverride ?? contextLocation;

let matches = React.useMemo(
() => matchRoutes(routes, location, basenameForMatching),
() => matchRoutes_(routes, location, basenameForMatching),
[location, routes, basenameForMatching]
);

if (!matches) {
// TODO: Warn about nothing matching, suggest using a catch-all route.
if (__DEV__) {
// TODO: Warn about nothing matching, suggest using a catch-all route.
}
return null;
}

Expand Down Expand Up @@ -595,8 +619,10 @@ export function createRoutesFromArray(
): RouteObject[] {
return array.map(partialRoute => {
let route: RouteObject = {
path: partialRoute.path || "/",
path: partialRoute.path || "",
caseSensitive: partialRoute.caseSensitive === true,
index: partialRoute.index === true,
// This is the same as default value of <Route element>
element: partialRoute.element || <Outlet />
};

Expand Down Expand Up @@ -637,8 +663,9 @@ export function createRoutesFromChildren(
}

let route: RouteObject = {
path: element.props.path || "/",
path: element.props.path || "",
caseSensitive: element.props.caseSensitive === true,
index: element.props.index === true,
// Default behavior is to just render the element that was given. This
// permits people to use any element they prefer, not just <Route> (though
// all our official examples and docs use <Route> for clarity).
Expand Down Expand Up @@ -670,6 +697,7 @@ export type Params = Record<string, string>;
export interface RouteObject {
caseSensitive: boolean;
children?: RouteObject[];
index: boolean;
element: React.ReactNode;
path: string;
}
Expand All @@ -679,11 +707,9 @@ export interface RouteObject {
* certain properties of a real route object such as `path` and `element`,
* which have reasonable defaults.
*/
export interface PartialRouteObject {
caseSensitive?: boolean;
export interface PartialRouteObject
extends Omit<Partial<RouteObject>, "children"> {
children?: PartialRouteObject[];
element?: React.ReactNode;
path?: string;
}

/**
Expand Down Expand Up @@ -716,6 +742,14 @@ export function matchRoutes(
location = parsePath(location);
}

return matchRoutes_(createRoutesFromArray(routes), location, basename);
}

function matchRoutes_(
routes: RouteObject[],
location: Partial<Location>,
basename = ""
): RouteMatch[] | null {
let pathname = location.pathname || "/";
if (basename) {
let base = basename.replace(/^\/*/, "/").replace(/\/+$/, "");
Expand Down Expand Up @@ -746,28 +780,41 @@ export interface RouteMatch {
}

function flattenRoutes(
routes: PartialRouteObject[],
routes: RouteObject[],
branches: RouteBranch[] = [],
parentPath = "",
parentRoutes: RouteObject[] = [],
parentIndexes: number[] = []
): RouteBranch[] {
(routes as RouteObject[]).forEach((route, index) => {
route = {
...route,
path: route.path || "/",
caseSensitive: !!route.caseSensitive,
element: route.element
};
routes.forEach((route, index) => {
let path: string;
if (route.path.startsWith("/")) {
invariant(
parentPath && !route.path.startsWith(parentPath),
`Absolute <Route path="${route.path}"> must begin ` +
`with its parent path "${parentPath}", otherwise it ` +
`will be unreachable.`
);

path = route.path;
} else {
path = joinPaths([parentPath, route.path]);
}

let path = joinPaths([parentPath, route.path]);
let routes = parentRoutes.concat(route);
let indexes = parentIndexes.concat(index);

// Add the children before adding this route to the array so we traverse the
// route tree depth-first and child routes appear before their parents in
// the "flattened" version.
if (route.children) {
if (route.children && route.children.length > 0) {
invariant(
!route.index,
`Index route for <Route path="${parentPath}"> must ` +
`not have children. If you want the route to be a layout ` +
`for its child routes, remove the \`index\` prop.`
);

flattenRoutes(route.children, branches, path, routes, indexes);
}

Expand All @@ -785,9 +832,7 @@ function rankRouteBranches(branches: RouteBranch[]): void {
return memo;
}, {});

// Sorting is stable in modern browsers, but we still support IE 11, so we
// need this little helper.
stableSort(branches, (a, b) => {
branches.sort((a, b) => {
let [aPath, , aIndexes] = a;
let aScore = pathScores[aPath];

Expand Down Expand Up @@ -843,13 +888,6 @@ function compareIndexes(a: number[], b: number[]): number {
0;
}

function stableSort(array: any[], compareItems: (a: any, b: any) => number) {
// This copy lets us get the original index of an item so we can preserve the
// original ordering in the case that they sort equally.
let copy = array.slice(0);
array.sort((a, b) => compareItems(a, b) || copy.indexOf(a) - copy.indexOf(b));
}

function matchRouteBranch(
branch: RouteBranch,
pathname: string
Expand Down

0 comments on commit 23a19c9

Please sign in to comment.