[typescript] Enable generic props for certain components #13868

pelotom · 2018-12-10T01:41:58Z

This revives the effort started in #11731, which ran into difficulty with callback parameter types no longer being inferred, thus breaking backwards compatibility. This new approach circumvents the issue by using overloads; in the simple case where no override component is provided, callback parameter types are fully inferred with no extra work. In the case where an override component is provided, you can either

provide the component type also as a generic parameter:

<Button<'div'>
  component="div"
  onClick={e => { /* e is inferred as MouseEvent<HTMLDivElement> */ }}
/>

provide an explicit type for the callback parameter which agrees with the component prop:

<Button
  component="div"
  onClick={(e: MouseEvent<HTMLDivElement>) => {}}
/>

So far in this PR only a handful of components have been converted to use this new typing strategy:

Avatar
Button
Fab
ListItem
Tabs

The results seem promising, but I wanted to solicit feedback before diving in too much deeper. One unfortunate issue is that error messages can be somewhat cryptic; for instance with

const Override = (props: {}) => <div />;

<Avatar component={Override} onClick={e => {}} />;

The error we get is that Property 'component' does not exist on type .... There is an error here, which is that we are providing a prop, onClick, which does not exist on the overriding component, however the message doesn't help us understand that.

Upgraded TypeScript to 3.2 because it contains a bugfix for microsoft/TypeScript#26004 which is necessary to make this work.

pelotom · 2018-12-10T01:43:43Z

packages/material-ui/src/TextField/TextField.spec.tsx

@@ -14,7 +14,8 @@ import TextField from '@material-ui/core/TextField';
      variant="standard"
      InputProps={{
        // notchedOutline is only used with variant "outlined"
-        classes: { inputTypeSearch: 'search-input', notchedOutline: 'notched-outline' }, // $ExpectError
+        // FIXME this no longer generates an error in TS 3.2, see https://github.com/Microsoft/TypeScript/issues/28926
+        // classes: { inputTypeSearch: 'search-input', notchedOutline: 'notched-outline' }, // $ExpectError


Upgrading TypeScript broke this test, due to microsoft/TypeScript#28926.

eps1lon · 2019-02-05T14:51:54Z

@pelotom I did commit some changes from my own review that I thought weren't that controversial. This includes some code comments for the types.

Could you go over some unaddressed review comments? I think we can start converting the other declarations and get some feedback on a alpha release.

pelotom · 2019-02-08T17:55:30Z

@eps1lon very sorry I've been AWOL on this, work has been crazy. I should have time to look this over tonight.

eps1lon · 2019-02-08T18:08:12Z

@eps1lon very sorry I've been AWOL on this, work has been crazy. I should have time to look this over tonight.

No worries. I think I only fixed some nitpicks I had. Looks like we can start applying this to the rest of the components which hopefully is pretty straight forward and doesn't break any tests.

pelotom · 2019-02-09T06:46:53Z

Looks like we can start applying this to the rest of the components which hopefully is pretty straight forward and doesn't break any tests.

I assume you mean merge this and then start to gradually migrate other components to the new system, yeah?

pelotom · 2019-02-09T06:48:53Z

packages/material-ui/test/typescript/components.spec.tsx

@@ -163,7 +163,7 @@ const BottomNavigationTest = () => {
  const value = 123;

  return (
-    <BottomNavigation value={value} onChange={e => log(e)} showLabels>


The purpose of writing these as lambdas was to ensure that the argument type was being inferred...

Shouldn't it complain that log is not assignable to (e: EventType) => ReturnType in those cases? What makes this different from the cases where you added an explicit ExpectType?

Shouldn't it complain that log is not assignable to (e: EventType) => ReturnType in those cases?

Perhaps it should, but it doesn't.

What makes this different from the cases where you added an explicit ExpectType?

It's a less rigorous test (just that something other than any was inferred for the argument, not making an assertion about what was inferred). There's no reason an ExpectType couldn't be added also.

Could you quickly push a test that is not throwing when it should for my own peace of mind? Just want to make sure we're not disabling some lint rule that was removed because of something unrelated.

Hm, I don't think I follow. Could you elaborate?

That doesn’t test inference because you’re passing a function whose signature is already determined. The point is to pass an inline function with no parameter signature so that if it can’t infer the type there will be a noImplicitAny error.

The point is to pass an inline function with no parameter signature so that if it can’t infer the type there will be a no-implicit-any error.

Ah now I understand. Happy to revert a7f6d4c then.

If we didn’t want to disable the lint rule we could just as well pass e => {} to test this, the log call is unimportant... but IIRC the only reason for it was to appease another lint rule for unused variables.

Probably should just selectively disable the lint rule for these lines.

I'm ok with disabling the rule since our ts files never make it to runtime anyway. I would say that the only useful pattern the lint rule flags is in react props. A new callback could defeat memoized components.

The ts files in our docs are transpiled to JS at which point eslint would cover that case.

If end up disabling the rules for every instance of unnecessary callbacks then we might as well disable it globally (or maybe starting with per file disables)

Ok, I reverted 79c4872.

This reverts commit a7f6d4c.

* Changed the Tab component to work with generic props (see #13868) * successfully run yarn prettier with the correct config * successfully ran yarn typescript

pelotom added 25 commits August 12, 2018 19:05

WIP

3d67416

Merge branch 'master' into typescript-generics-new-approach

9ba3309

WIP

070df8f

Upgrade TypeScript

dbf061b

Rename defaultRoot to defaultComponent

3e65e4e

Fix InnerProps operator

6a5f2a4

Add comments to type tests

66f1885

Add another test

bc82b64

Remove unnecessary generic

29c8771

Format comment

2f3d066

Tweak comment

d2f84ea

Use PropsOf definition from master

a1de8af

Move AnyReactType to muiComponent

95f1ff3

Renames

a24d8af

Merge branch 'master' into typescript-generics-new-approach

8935391

Upgrade TypeScript

e68aae8

Comment out test that doesn't work in TS 3.2

95ad1ad

Fix some errors

0ad90c1

Merge branch 'upgrade-ts' into typescript-generics-new-approach

e2470e8

Use SimplifiedPropsOf to obtain exported Props type for each component

2dd8b56

Add error expectations

e6acb1d

Test callback arg types

fc402c4

Fix derivatives of ButtonBase

4077fee

Make an ExtendButtonBase utility

cfe50c2

Merge branch 'master' into typescript-generics-new-approach

b58e667

pelotom requested a review from eps1lon December 10, 2018 01:41

pelotom changed the title ~~Typescript generics (new approach)~~ Generic components (new approach) Dec 10, 2018

pelotom changed the title ~~Generic components (new approach)~~ [typescript] Generic components (new approach) Dec 10, 2018

pelotom added the typescript label Dec 10, 2018

pelotom commented Dec 10, 2018

View reviewed changes

eps1lon added 5 commits February 5, 2019 15:10

[typescript] Drop PropsOf in favor of React.ComponentProps

f5ccac1

[test] Move var declaration closer to callsite

1a179f6

Merge branch 'next' into pr/pelotom/13868

c080e5f

[chore] format

b5edd2a

[typescript] add some code docs to overrideable component types

752c8e6

eps1lon mentioned this pull request Feb 5, 2019

Incorrect TypeScript type definition change for InputLabel #14409

Closed

2 tasks

eps1lon self-assigned this Feb 8, 2019

eps1lon added 2 commits February 8, 2019 17:03

Merge branch 'next' into pr/pelotom/13868

6b95cbf

[typescript] explain ExtendButtonBase

06a7bf5

eps1lon approved these changes Feb 8, 2019

View reviewed changes

eps1lon changed the title ~~[typescript] Generic components (new approach)~~ [typescript] Enable generic props for certain components Feb 8, 2019

[core] hoist dev dependencies

e5990ed

pelotom commented Feb 9, 2019

View reviewed changes

pelotom added 2 commits February 8, 2019 23:24

Allow Simplify to distribute over unions

b02652f

Revert "[lint] Re-enable no-unecessary-callbackwrapper"

79c4872

This reverts commit a7f6d4c.

pelotom merged commit 54f9a33 into mui:next Feb 10, 2019

pelotom deleted the typescript-generics-new-approach branch February 10, 2019 05:30

eps1lon mentioned this pull request Feb 10, 2019

[typescript] Add type test for style lib interopability #14482

Merged

johnrichter mentioned this pull request Feb 19, 2019

Derivative Shorthand Styled Components Lose Override Typing #14586

Closed

2 tasks

caroe233 pushed a commit to caroe233/material-ui that referenced this pull request Mar 21, 2019

Changed the Tab component to work with generic props (see mui#13868)

5b0cdee

caroe233 mentioned this pull request Mar 22, 2019

[Tab] Enable generic props #15003

Merged

1 task

eps1lon pushed a commit that referenced this pull request Mar 23, 2019

[Tab] Enable generic props (#15003)

7ee2a08

* Changed the Tab component to work with generic props (see #13868) * successfully run yarn prettier with the correct config * successfully ran yarn typescript

eps1lon mentioned this pull request Mar 25, 2019

[ListItem] Add type test for button prop #15049

Merged

MastroLindus mentioned this pull request Mar 29, 2019

[Typescript] component prop on listitem doesn't allow you to specify component as div and button as false #15106

Closed

alexanderchan mentioned this pull request Feb 25, 2020

Create the menu and menuitem components in kodiak ui godaddy-wordpress/kodiak-ui#36

Merged

6 tasks

NMinhNguyen mentioned this pull request May 25, 2020

[docs] Update minimum TypeScript version to 3.2 #21197

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript] Enable generic props for certain components #13868

[typescript] Enable generic props for certain components #13868

pelotom commented Dec 10, 2018 •

edited

Loading

pelotom Dec 10, 2018

eps1lon commented Feb 5, 2019

pelotom commented Feb 8, 2019

eps1lon commented Feb 8, 2019

pelotom commented Feb 9, 2019

pelotom Feb 9, 2019

eps1lon Feb 9, 2019

pelotom Feb 9, 2019

eps1lon Feb 9, 2019

pelotom Feb 9, 2019

pelotom Feb 9, 2019 •

edited

Loading

eps1lon Feb 9, 2019

pelotom Feb 9, 2019 •

edited

Loading

eps1lon Feb 9, 2019

pelotom Feb 9, 2019

[typescript] Enable generic props for certain components #13868

[typescript] Enable generic props for certain components #13868

Conversation

pelotom commented Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

eps1lon commented Feb 5, 2019

pelotom commented Feb 8, 2019

eps1lon commented Feb 8, 2019

pelotom commented Feb 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom commented Dec 10, 2018 •

edited

Loading

pelotom Feb 9, 2019 •

edited

Loading

pelotom Feb 9, 2019 •

edited

Loading