[react-router] Fix #38271 (withRouter fails with ComponentType starting in 3.6.2)

[karol-majewski]

Fixes #38271.

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
• If you are making substantial changes, consider adding a 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.

you should strive to have fewer type parameters in your function signatures

https://twitter.com/SeaRyanC/status/1118632999266861056

[typescript-bot]

@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 Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[typescript-bot]

👋 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?

	master	#38326	diff
Batch compilation
Memory usage (MiB)	168.5	170.7	+1.3%
Type count	69097	69184	+0.1%
Assignability cache size	60950	61560	+1.0%
Subtype cache size	1030	1026	-0.4%
Identity cache size	111	107	-3.6%
Language service
Samples taken	777	777	0.0%
Identifiers in tests	1879	1898	+1.0%
getCompletionsAtPosition
Mean duration (ms)	980.1	1053.6	+7.5%
Median duration (ms)	901.3	1185.0	+31.5% 🔸
Mean CV	6.8%	6.6%	-3.5%
Worst duration (ms)	1234.7	1343.7	+8.8%
Worst identifier	div	Route
getQuickInfoAtPosition
Mean duration (ms)	1119.5	1054.8	-5.8%
Median duration (ms)	1128.6	1144.0	+1.4%
Mean CV	5.9%	6.6%	+11.9%
Worst duration (ms)	1335.2	1378.4	+3.2%
Worst identifier	transitionEnterTimeout	props

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 @andrewbranch. Have a nice day!

[eps1lon]

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?

[karol-majewski]

Though I don't know what this has to do with the tweet.

The previous signature for withRouter was using too many type parameters. It was using two, where in reality there is just one. Because the component (C) and its props (P) were defined separately, they were treated separately, causing the compiler to throw an error in perfectly valid code.

Notice the dubious part of the old definition:

component: C & React.ComponentType<P>,

We already know C is a subtype of React.ComponentType<P>. Why would one create such a useless intersection?

Removing the & React.ComponentType<P> bit uncovers the answer: it was hiding a type error. Without it, the tests fail. As they should, because the definition was never correct.

Curious why this would result in slower benchmarks.

I'm curious, too! Some metrics went up, some went down. Not sure how to interpret it.

Could you try returning a plain function instead?

Do you mean returning a React.FunctionComponent instead of React.ComponentClass? Well, that depends on what the library is actually doing in runtime. I would rather not change it without making sure this reflects the runtime reality first.

[eps1lon]

Didn't spot the previous second type parameter, sorry.

Do you mean returning a React.FunctionComponent instead of React.ComponentClass?

No a plain function e.g. (props: Props) => JSX.Element

Well, that depends on what the library is actually doing in runtime.

That's a common misconception. We implement the types (the interface) not represent the implementation. If withRouter only ever documents the usage as a component then any usage relying on it being a class or function is invalid.

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.

[karol-majewski]

No a plain function e.g. (props: Props) => JSX.Element

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 withRouter supports both types of components, but also makes a change to the return type based on the type of the provided component:

DefinitelyTyped/types/react-router/index.d.ts

Lines 134 to 136 in bf75bcd

	export type WithRouterProps<C extends React.ComponentType<any>> = C extends React.ComponentClass
	? { wrappedComponentRef?: React.Ref<InstanceType<C>> }
	: {};

[eps1lon]

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

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.

ref being available is simply a matter of the props interface not the component type. You can define function components that accept a ref.

[typescript-bot]

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!

[orta]

OK, looks good to merge to me then yep 👍

[typescript-bot]

I just published @types/react-router@5.0.4 to npm.

[ulrichb]

Hmmm ... this makes type inference fail when using

withRouter(React.memo<RouteComponentProps & {
    readonly someProp: string;
}>( ({ match, someProp }) =>  /* ... */ ) ))

:(

[ksylvest]

Looks like this definition now fails:

const Sample: React.FC<RouteComponentProps & { name: string }> = ({ name }) => (
  <>{name}</>
);

const SampleWithRouter = withRouter(Sample);

[eps1lon]

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 strictFunctionTypes and I have a solution in mind. We'll see if it works out.

[artola]

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

https://www.typescriptlang.org/play/index.html?target=6&jsx=2#code/JYWwDg9gTgLgBAbwAoFcoFMDCFwQHbp4wA0cAYingMYzD7a4FEC+cAZlDnAOQYCGNbgG4AUKEixEAJQgoYWHJCYwknMAGdSAd2AwAFjLnoorDl17oBMALScjUawBMcwkSKoAbPuvXkIEOHQAD3k8R19UDAYlQhgAHhE4OEN5aPxY1QgNOIQwPig+EAAuOHUYKGA8AHNmAD4RWsREuAww4wAKAEompKSMGDQ8ODwUDw9RJOYRKfd8MrgAIXySimpaekV0ojiUhUYMtXUcvILi0vLKmtrGgF44LrgbxpGx0REdfV2odrJ-TrePgZZPJvksoP8REA