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
[react-router] Fix #38271 (withRouter
fails with ComponentType
starting in 3.6.2)
#38326
[react-router] Fix #38271 (withRouter
fails with ComponentType
starting in 3.6.2)
#38326
Conversation
Co-authored-by: Oliver Joseph Ash <oliverjash@gmail.com>
@karol-majewski Thank you for submitting this PR! 🔔 @sergey-buturlakin @mrk21 @vasek17 @ngbrown @awendland @KostyaEsmukov @johnnyreilly @LKay @DovydasNavickas @huy-nguyen @Grmiade @DaIgeb @egorshulga @rraina @pret-a-porter @t49tran @8enSmith @wezleytsai @eps1lon @HipsterBrown - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics. Let’s review the numbers, shall we?
Looks like there were a couple significant differences—take a look at median duration for getting completions at a position to make sure everything looks ok. If you have any questions or comments about me, you can ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine. Though I don't know what this has to do with the tweet. That one was talking about type parameters not union return types.
Curious why this would result in slower benchmarks. Could you try returning a plain function instead?
The previous signature for Notice the dubious part of the old definition: component: C & React.ComponentType<P>, We already know Removing the
I'm curious, too! Some metrics went up, some went down. Not sure how to interpret it.
Do you mean returning a |
Didn't spot the previous second type parameter, sorry.
No a plain function e.g.
That's a common misconception. We implement the types (the interface) not represent the implementation. If The reason I'm asking for a plain function is that it's the type with the least amount of implied types. Especially implicit children cause many issues with react and typescript. Since we never gave the type guarantee that the component is a class we can safely choose the simplest type. |
As my colleague @OliverJAsh pointed out, such a change would break the callers relying on features available only on class-based components (like React refs). This piece of code indicates that not only DefinitelyTyped/types/react-router/index.d.ts Lines 134 to 136 in bf75bcd
|
Just realized we never returned a ComponentType. Should be fine. Regardless of this change I would recommend to not rely on the specific component type declared in typescript.
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
OK, looks good to merge to me then yep 👍 |
I just published |
Hmmm ... this makes type inference fail when using withRouter(React.memo<RouteComponentProps & {
readonly someProp: string;
}>( ({ match, someProp }) => /* ... */ ) )) :( |
Looks like this definition now fails: const Sample: React.FC<RouteComponentProps & { name: string }> = ({ name }) => (
<>{name}</>
);
const SampleWithRouter = withRouter(Sample); |
This comment has been minimized.
This comment has been minimized.
I encourage everyone to open new issues. If we fix your particular issue you have no way of being notified if you only comment in a closed thread. The typescript playground now allows configuring the ts version + config and is able to download type definitions. Including a link in a new issue helps maintainers a lot. Thank you 🙏 It seems like this only throws with |
@orta From my point of view the offending new types need to be rolled back, and the newest should have a simple test to verify them. |
* Revert implementation of #38326 * Skip ComponentType test which only fails starting with 3.6
Fixes #38271.
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
tslint.json
containing{ "extends": "dtslint/dt.json" }
. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.No substantial changes are made here. This PR fixes a type error uncovered by TypeScript 3.6.2 by using 1 type parameter instead of two. The second one is derived with
React.ComponentProps
.https://twitter.com/SeaRyanC/status/1118632999266861056