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(react): Update types to match react router 6.4 updates #5992

Merged
merged 3 commits into from Oct 20, 2022

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Oct 19, 2022

Supersedes #5915'
Fixes #5959

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.

@AbhiPrasad AbhiPrasad self-assigned this Oct 19, 2022
@AbhiPrasad AbhiPrasad requested review from a team, mydea, lobsterkatie and Lms24 and removed request for a team October 19, 2022 09:24
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM, out of curiosity, you said that it's not very easy to maintain backwards compatiblity if we didn't use any but a stronger type. What would we need to do to get that stronger type?

Not saying it needs to be done (consider this a logaf extra-l) but since we try to avoid any as much as possible, I was just curious.

// This type was originally just `type RouteObject = IndexRouteObject`, but this was changed
// in https://github.com/remix-run/react-router/pull/9366, which was released with `6.4.2`
// See https://github.com/remix-run/react-router/issues/9427 for a discussion on this.
type RouteObject = IndexRouteObject | NonIndexRouteObject;
Copy link
Member

@mydea mydea Oct 20, 2022

Choose a reason for hiding this comment

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

L: I guess to reduce duplication this could also be something like:

interface BaseRouteObject {
  caseSensitive?: boolean;
  element?: React.ReactNode | null;
  path?: string;
}

type IndexRouteObject = BaseRouteObject & { index: true, children?: undefined };
type NonIndexRouteObject = BaseRouteObject & { index: false, children?: RouteObject[]; };
type RouteObject = IndexRouteObject | NonIndexRouteObject;

But not sure if it is really worth it, or just adds unnecessary complexity 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I just kept the types from what was done in #5915, you're change is cleaner, but I'll just keep it like this for now. We can update this afterwards if we need to.

@AbhiPrasad
Copy link
Member Author

LGTM, out of curiosity, you said that it's not very easy to maintain backwards compatiblity if we didn't use any but a stronger type. What would we need to do to get that stronger type?

The problem here is that with TS 3.8.3, TypeScript isn't smart enough to discriminate the union RouteObject type. There might be a way to make this work, but I figured just slamming any with type casts is good enough. We have plenty of tests for different behavior here, so we should be able to still make this work.

@AbhiPrasad AbhiPrasad merged commit 6e70534 into master Oct 20, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-update-react-router-types branch October 20, 2022 11:42
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.

matchRoutes type is incompatible with routes parameter in react-router integration
4 participants