fix: Strengthen route typings #9366
Conversation
- RouteObject type should accept either index or children.
🦋 Changeset detectedLatest commit: 7bee9dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -220,49 +223,28 @@ export function Outlet(props: OutletProps): React.ReactElement | null { | |||
| return useOutlet(props.context); | |||
| } | |||
|
|
|||
| interface DataRouteProps { | |||
There was a problem hiding this comment.
Got rid of the duplication here - <Route> props are now just RouteObject equivalents that expect ReactNode children instead of ReactElement children
|
Note to self based on a tangential conversation with Michael - we probably need to move some |
| export type AgnosticIndexRouteObject = AgnosticBaseRouteObject & { | ||
| children?: undefined; | ||
| index: true; | ||
| }; | ||
|
|
||
| /** | ||
| * Non-index routes may have children, but cannot have index | ||
| */ | ||
| export type AgnosticNonIndexRouteObject = AgnosticBaseRouteObject & { | ||
| children?: AgnosticRouteObject[]; | ||
| index?: false; | ||
| }; |
There was a problem hiding this comment.
These are the new foundation of the types. They get extended in react-router with element/errorElement and then again in RouteProps to change children from RouteObject to ReactNode
|
|
||
| export interface RouteProps extends DataRouteProps { | ||
| caseSensitive?: boolean; | ||
| export type PathRouteProps = Omit<NonIndexRouteObject, "children"> & { |
There was a problem hiding this comment.
Just a nit here: but I much prefer to avoid using Omit if we can and avoid the mental overhead of looking up that interface when reading through this code. Listing out all properties explicitly means it's all right here in front of me.
| id?: AgnosticIndexRouteObject["id"]; | ||
| loader?: AgnosticIndexRouteObject["loader"]; | ||
| action?: AgnosticIndexRouteObject["action"]; | ||
| hasErrorBoundary: AgnosticIndexRouteObject["hasErrorBoundary"]; |
There was a problem hiding this comment.
@brophdawg11 I'm a bit late to the party, but better late than never: while testing the prerelease for #9352 I noticed this property had suddenly become required. I'm assuming this might be unintentional? 😊 Same goes for NonIndexRouteObject below.
There was a problem hiding this comment.
Oh thank you! Yes that's definitely a typo :)
Fixes a type mismatch caused by change remix-run/react-router#9366 in react-router code.
Fixes a type mismatch caused by change remix-run/react-router#9366 in react-router code.
Fixes a type mismatch caused by change remix-run/react-router#9366 in react-router code. Have to use any because of typing changes in the above PR, since we can't be backwards compatible with both versions very easily. Co-authored-by: Philip Atkinson <[email protected]>
| shouldRevalidate?: AgnosticIndexRouteObject["shouldRevalidate"]; | ||
| handle?: AgnosticIndexRouteObject["handle"]; | ||
| index: true; | ||
| children?: undefined; |
There was a problem hiding this comment.
why children?: undefined;, maybe we can remove this line?
Based off #9343 and extends route type safety downwards to
@remix-run/routerand upwards to<Route>prop typings. This provides type-safety for JSX-defined route trees:Note: screenshots are slightly outdated, spoke to @mjackson and it is valid to have a path on index routes, and serves as a way to separate index from layout routes. This PR has been update to only prevent
index+children.As well as manually defined route trees:
This also adds a few invariants and associated tests to check for these invalid conditions:
flattenRoutes(lowest common denominator)createRoutesFromChildren(always used inBrowserRouter, sometimes used inRouterProvider)convertRoutesToDataRoutes(always used inRouterProvider)