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

Allow nested absolute paths #7992

Merged
merged 2 commits into from
Sep 10, 2021
Merged

Allow nested absolute paths #7992

merged 2 commits into from
Sep 10, 2021

Conversation

mjackson
Copy link
Member

@mjackson mjackson commented Sep 1, 2021

This PR allows child routes with absolute paths that match the combined path of all their parent routes. Child routes with absolute paths that do not match their parent routes are an error.

This should make it a little easier for people who prefer to define their route paths in a separate file of constants to do so without having to convert them to relative paths when they create their route config, like:

const USERS_PATH = "/users";
const USER_PROFILE_PATH = `${USERS_PATH}/:id`;

let routes = [
  {
    path: USERS_PATH,
    element: <Users />,
    children: [
      { path: USER_PROFILE_PATH, element: <UserProfile /> }
    ]
  }
];

This PR also makes the following improvements:

  • Add optional <Route index> prop that throws if that route has children. This doesn't really affect the execution of the code, but should make it easier to scan and a little safer
  • Loosens up the RouteObject type so that every property is optional (same as with <Route>'s props. This eliminates the need for the PartialRouteObject interface, so it's gone
  • Returns the same route object from matchRoutes on each match.route as the one that was passed in. This should make it easier for developers to pass through custom properties in their route objects
  • Remove createRoutesFromArray utility method. You can now just use your array directly in matchRoutes instead of running it through createRoutesFromArray first

Fixes #7335

@MeiKatz
Copy link
Contributor

MeiKatz commented Sep 2, 2021

Counter proposal for index: instead of <Route path="" /> or <Route /> to match all sub-routes, use <Route path="*" />. Then you can use <Route path="" /> and <Route /> for the index route. I get the point why there is a need for something like index, but that seems to me like some sort of <IndexRoute /> like we had in RRv3 and that was dropped for good reasons.

@mjackson
Copy link
Member Author

mjackson commented Sep 2, 2021

The index prop isn’t required for an index route. It just makes the code easier to scan at a glance and throws if that route has children, so it provides an extra guarantee that someone won’t come along later and add children.

@MeiKatz
Copy link
Contributor

MeiKatz commented Sep 2, 2021

So it's just an "useless" variable that adds meaning to the code but doesn't change anything in the program?

Also:

- Add (optional) `<Route index>` prop for index routes
- Return original route objects from `matchRoutes`
- Remove `PartialRouteObject` interface, use `RouteObject` instead
- Remove `createRoutesFromArray`

Fixes #7335
@mjackson
Copy link
Member Author

adds meaning to the code but doesn't change anything in the program?

@MeiKatz Yeah, kind of. The only thing it changes in the program itself is it will throw if an index route has children. This should make index routes a little safer to use, knowing someone won't come along an inadvertently add children to them in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants