[typings] Switch tabIndex from string type to number | string

[xaviergonz]

Basically this fixes an inconsistency where some tabIndex props where numbers in some components and others were mistakenly typed as string (since the tabIndex type in the react official types is number, not string.

[oliviertassinari]

Looking at JedWatson/react-select#1595 I think that we should allow both string and integer type but using integer internally in the lib.

[oliviertassinari]

I think that we should be updating the typescript definition too.

[xaviergonz]

so should I change them all to be string | number then?
Btw, are you referring to ts type defs? I changed those too

[oliviertassinari]

My bad, the typescript are already in sync 👍

[oliviertassinari]

Yes, I think that it would be better with string | number :).

ok

[oliviertassinari]

@xaviergonz Great!

[xaviergonz]

@oliviertassinari The latest release doesn't compile with TS due to this bug :(

node_modules/material-ui-next/Chip/Chip.d.ts(4,18): error TS2430: Interface 'ChipProps' incorrectly extends interface 'HTMLAttributes<HTMLDivElement>'.
failed with code 2
Types of property 'tabIndex' are incompatible.
Type 'string | number | undefined' is not assignable to type 'number | undefined'.
Type 'string' is not assignable to type 'number | undefined'.

basically it is complaining that the react type (number | undefined) and the new one (number | string | undefined) are not compatible

Should I change it back to number | undefined for that particular case or them all?

Another possibility would be to hand pick properties from HTMLAttributes that might be needed, but I don't know which those would be.

[oliviertassinari]

@xaviergonz I would rather follow React.HTMLAttributes<HTMLDivElement> typing for typescript. But isn't React.HTMLAttributes<HTMLDivElement> typing wrong?

[xaviergonz]

I don't see how they would be, they are the official ones made by react guys. IMHO it is better to use a number, since actually tabIndex has to be a number (even when transformed in the end internally to a string attribute).

I guess they did it because that way you cannot mistype it as tabIndex="hello world" (which with material-ui right now is accepted but wrong), but instead have to type it as tabIndex={1234}

[xaviergonz]

Also it is more natural for TS users, since we are used to do this (due to react official typings):
<div tabIndex={1234}/> since <div tabIndex="1234"/> gives you a type warning

[oliviertassinari]

The following change for the typescript files should fix it then :)

-  tabIndex?: number | string;
+  tabIndex?: number;

[xaviergonz]

I'll change them then on a new pull request :)

[oliviertassinari]

Does it require a new release? If it does, I will wait a day or two to collect more feedback.

[xaviergonz]

It would for TS users or they get compilation errors

[wcandillon]

@oliviertassinari I would like a release for this fix please 🙋🏼‍♂️

[evolify]

Me too, the same problem when use typescript.

[oliviertassinari]

I will cut a release this evening.