fix(react-router): Failing with strictFunctionTypes

[eps1lon]

Resolves issues reported in #38271 (comment) and #38326 (comment)

I'll try to make sense of the current issue later in this text but TL;DR: The introduced bug reports have more upvotes than the fixed bug reports so we revert until we figure out what's happening.

I tried to fiddle around with the withRouter declaration but anything I tried created an error in a different version of TS with strictFunctionTypes.

The current issue is a result of C & ComponentType<P> which resolves (in the removed test case) to FunctionComponent<P> | (Functioncomponent<P> & ComponentClass<P>) which is probably a result of the intermediate distribution of & to (FunctionComponent<P> & FunctionComponent<P>) | (FunctionComponent<P> & ComponentClass<P>) | (ComponentClass<P> & FunctionComponent<P>) | (FunctionComponent<P> & ComponentClass<P>) which should be simplified to FunctionComponent<P> | ComponentClass<P> | (FunctionComponent<P> | ComponentClass<P>) but for some reason the ComponentClass<P> is dropped. Maybe TypeScript is using only the first part of the union and resolves the C in C & ComponentType<P> to FunctionComponent<P> & ComponentType<P> which would explain the result.

[typescript-bot]

@eps1lon 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 @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]

Since you're a listed owner and the build passed, this PR is fast-tracked. A maintainer will merge 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!

[OliverJAsh]

If we merge this can we make sure we re-open or re-file #38271 so it's not lost?

[eps1lon]

If we merge this can we make sure we re-open or re-file #38271 so it's not lost?

Yes. I probably have another fix for this and suspect it's actually an issue with TypeScript. I'll narrow it down a bit more and keep you posted (likely pinging you in a TS issue).

[nstepien]

Shouldn't we add another test to prevent future regressions?
In my case the new types break on a component that looks like this:

const Component = withRouter(({ a, b, location, ...props }: ComponentOwnProps & RouteComponentProps) => {
  // ...
});

[OliverJAsh]

Thanks for opening microsoft/TypeScript#33490

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

Comparison details 📊

	master	#38453	diff
Batch compilation
Memory usage (MiB)	167.2	161.4	-3.5%
Type count	69184	69115	-0.1%
Assignability cache size	61560	60966	-1.0%
Subtype cache size	1026	1030	+0.4%
Identity cache size	107	113	+5.6%
Language service
Samples taken	777	777	0.0%
Identifiers in tests	1898	1891	-0.4%
getCompletionsAtPosition
Mean duration (ms)	1051.8	1082.6	+2.9%
Median duration (ms)	1148.2	1166.0	+1.6%
Mean CV	9.1%	7.3%	-20.3%
Worst duration (ms)	1354.4	1344.5	-0.7%
Worst identifier	image	Link
getQuickInfoAtPosition
Mean duration (ms)	1097.7	1061.3	-3.3%
Median duration (ms)	1130.8	1137.0	+0.5%
Mean CV	8.2%	7.4%	-9.7%
Worst duration (ms)	1372.0	1330.4	-3.0%
Worst identifier	props	props

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

---

If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

[eps1lon]

Shouldn't we add another test to prevent future regressions?
In my case the new types break on a component that looks like this:

const Component = withRouter(({ a, b, location, ...props }: ComponentOwnProps & RouteComponentProps) => {
  // ...
});

I'll check it out later. I think main issue that caused this regression was that the tests do not run with strictFunctionTypes. Could you check if the regression persists after disabling this option in your tsconfig?

[nstepien]

"strictFunctionTypes": false does remove the error.
Shouldn't types be tested with "strict": true as well?

[eps1lon]

"strictFunctionTypes": false does remove the error.
Shouldn't types be tested with "strict": true as well?

The default for @types/ packages does not include this, no. I'm not sure about the implications but I suspect it would require package consumer to enable this as well to be sure they get working types. For example since all types are tested with strictNullChecks they might behave differently without it. Package consumers without that option basically use untested types.

[nstepien]

I would have expected the opposite: strict types work with loose settings, while loose types will break with strict settings.

[orta]

👍 looks good to me, we should get this fix in.

I think it might be worth moving to have strict: true also, because strict TS is effectively a subset of loose TS which is effectively a subset of loose JS 🍡

[typescript-bot]

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

[eps1lon]

I think it might be worth moving to have strict: true also, because strict TS is effectively a subset of loose TS which is effectively a subset of loose JS dango

I need to revisit how we implemented the type helper I have in mind but one issue with strictNullChecks is that conditionals can behave differently. This means you can create different types with e.g. undefined extends T ? U : never. So you have to be very cautious with every conditional you use and consider different configurations.