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: Strengthen route typings #9366

Merged
merged 16 commits into from
Sep 30, 2022
Merged

fix: Strengthen route typings #9366

merged 16 commits into from
Sep 30, 2022

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Sep 28, 2022

Based off #9343 and extends route type safety downwards to @remix-run/router and 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.

Screen Shot 2022-09-28 at 12 02 14 PM

As well as manually defined route trees:

Screen Shot 2022-09-28 at 12 29 34 PM

This also adds a few invariants and associated tests to check for these invalid conditions:

  • flattenRoutes(lowest common denominator)
  • createRoutesFromChildren (always used in BrowserRouter, sometimes used in RouterProvider)
  • convertRoutesToDataRoutes (always used in RouterProvider)

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2022

🦋 Changeset detected

Latest commit: 7bee9dc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
react-router Patch
react-router-dom Patch
react-router-native Patch
@remix-run/router Patch
react-router-dom-v5-compat Patch

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of the duplication here - <Route> props are now just RouteObject equivalents that expect ReactNode children instead of ReactElement children

@brophdawg11
Copy link
Contributor Author

Note to self based on a tangential conversation with Michael - we probably need to move some invariant's into matchRoutes as well here. Although it looks like we have one in flattenRoutes... Need to dig into that a bit more

packages/router/utils.ts Outdated Show resolved Hide resolved
Comment on lines +154 to +165
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;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just had one small nit about code readability. Thanks 🙏


export interface RouteProps extends DataRouteProps {
caseSensitive?: boolean;
export type PathRouteProps = Omit<NonIndexRouteObject, "children"> & {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@brophdawg11 brophdawg11 merged commit 540f94b into dev Sep 30, 2022
@brophdawg11 brophdawg11 deleted the brophdawg11/route-typings branch September 30, 2022 14:49
id?: AgnosticIndexRouteObject["id"];
loader?: AgnosticIndexRouteObject["loader"];
action?: AgnosticIndexRouteObject["action"];
hasErrorBoundary: AgnosticIndexRouteObject["hasErrorBoundary"];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thank you! Yes that's definitely a typo :)

philipatkinson added a commit to philipatkinson/sentry-javascript that referenced this pull request Oct 7, 2022
Fixes a type mismatch caused by change remix-run/react-router#9366 in react-router code.
AbhiPrasad pushed a commit to getsentry/sentry-javascript that referenced this pull request Oct 19, 2022
Fixes a type mismatch caused by change remix-run/react-router#9366 in react-router code.
AbhiPrasad added a commit to getsentry/sentry-javascript that referenced this pull request Oct 20, 2022
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 <philipatkinson@users.noreply.github.com>
shouldRevalidate?: AgnosticIndexRouteObject["shouldRevalidate"];
handle?: AgnosticIndexRouteObject["handle"];
index: true;
children?: undefined;
Copy link

@lilonghe lilonghe Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why children?: undefined;, maybe we can remove this line?

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

Successfully merging this pull request may close these issues.

7 participants