Skip to content

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 added the Package: react Issues related to the Sentry React SDK label Oct 19, 2022
@AbhiPrasad AbhiPrasad requested review from a team, Lms24, lobsterkatie and mydea 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

Labels

Package: react Issues related to the Sentry React SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

5 participants