Add TypeScript generics for component props

[nickserv]

This should allow strongly typing extra props derived from component props, such as using the to prop of React Router's Link component on ButtonBase components. Prop interfaces that include a component prop with prop spreading now have a generic type with no constraint that default to an empty interface. This also makes it possible to use TypeScript 2.9's new JSX generics when they can't be inferred.

Examples

<Button component={Link} to="/open-collective">Link</Button>
<Button<LinkProps> component={Link} to="/open-collective">Link</Button>

Related

• Missing typescript attribute for Button component #7877
• [Button] Add react-router link support in typescript #8043
• [typescript] Smarter logic? #8063
• Question on flow support #9109

[nickserv]

I'm trying to figure out why this example doesn't trigger a compiler error. It seems like TypeScript is implying the generic type C to be any which is not desired for type safety, but I don't know how to work around it. I'd appreciate any ideas.

<Button to="/open-collective">Link</Button>

[oliviertassinari]

Right now, the encouraged approach is:

const MyLink = props => <Link {...props} to="/open-collective" />

<Button component={MyLink}>Link</Button>

[nickserv]

That's definitely an improvement over what I was using, though I like the type safety and convenience of directly checking props on the component's generic type without having to create a separate component to work around TypeScript. Also with @oliviertassinari's example it seems like you have to import { ListItemProps } from '@material-ui/core/ListItem' because the props aren't included in index.d.ts, it would be nice to export them if we don't take the generic approach.

[nickserv]

Actually I am having an issue with that approach, the following code errors because ListItem uses a string ref while Link uses a function ref for innerRef: const NameLink = (props: ListItemProps) => <Link to={name} {...props} />

Is there a way to strongly type this without generics (not using any)?

[franklixuefei]

What's the status of this PR? Seems related to #11921

[pelotom]

It's a fairly extensive change and I've been too busy to give it the attention it deserves, might get some time to review this weekend. Looks like it has conflicts currently though.

[nickserv]

This should be up to date now.

[pelotom]

This is really promising! I wasn't aware that inference of component type arguments would be good enough to make this kind of approach work.

[pelotom]

It looks like the generic argument can actually be left off here and inference works fine:

-    <Button<LinkProps> component={Link} to="/open-collective">
+    <Button component={Link} to="/open-collective">
      Link
    </Button>
-    <Button<LinkProps> to="/open-collective">Link</Button>
+    <Button to="/open-collective">Link</Button>

[pelotom]

It seems like what we actually want here (and similarly for other components) is something more like

export interface AvatarProps<C = React.HTMLAttributes<HTMLDivElement>>
    extends StandardProps<C, AvatarClassKey> & {

I.e. in each case take the type we used to be passing as the first argument to StandardProps, make that the default for C, and then pass C to StandardProps instead. There's a problem though: we can no longer use an interface because we're intersecting with a type variable, so we have to switch to a type alias and intersection:

export type AvatarProps<C = React.HTMLAttributes<HTMLDivElement>>
    = StandardProps<C, AvatarClassKey> & {

Also StandardProps does the work of &-ing that first argument with the rest of your props, so you don't need to do it in the declare statement:

declare class Avatar<C> extends React.Component<AvatarProps<C>> {}

This approach looks promising at first blush, but it doesn't quite work ideally because the default type argument isn't used when you use a bare component, i.e. this fails:

<Avatar title="hello" /> // `title` from HTMLAttributes is invalid

You have to provide an explicit type parameter to fix it:

<Avatar<React.HTMLAttributes<HTMLDivElement>> title="hello" />

This would be a huge breaking change and is just gross 😕

But conditional types to the rescue! This seems to work:

export type AvatarProps<C = {}> = StandardProps<
  {} extends C ? React.HTMLAttributes<HTMLDivElement> : C,
  AvatarClassKey
> & {

Basically, if C is not inferred as anything specific, either because you referenced bare AvatarProps or because you didn't provide a component prop to Avatar to allow it to disambiguate, you will get the prop types that match the default component, in this case a div.

Now we can just write

<Avatar title="hello" />

and importantly, this doesn't work, which it shouldn't!

<Avatar
  component={(props: {/* no title prop! */}) => <div />}
  title="hello"
/>

What do you think?

[pelotom]

I tried out the above strategy on a local copy of this branch and it almost works, but there's a pretty unfortunate limitation: it turns out TypeScript can't infer the parameter types of callback props associated with the inner component. So for example, all the basic event handler props e.g. onClick associated with a default component of div would no longer be able to infer the parameter type and would require annotation.

I've filed a TypeScript ticket and asked a question on SO about it.

[nickserv]

Thanks for your research and suggestions. Is that a back compatibility breaking issue that would need to be fixed before we continue?

[pelotom]

@nickmccurdy

Is that a back compatibility breaking issue that would need to be fixed before we continue?

Yes, it would be backwards-breaking. To make it concrete, currently you can write this

<Button onClick={e => console.log(e.target)}>
  Click me!
</Button>

and TypeScript infers the type of the e parameter just fine. But with this change it would give up and infer e: any unless you provide an explicit annotation like

<Button onClick={(e: React.MouseEvent<HTMLElement>) => console.log(e.target)}>
  Click me!
</Button>

On the plus side if you have noImplicitAny enabled it would give you an error if you provide anything but the correct type annotation, which is more than can be said for the current situation, but it's quite a bit less convenient 😕

[oliviertassinari]

How do you guys want to move forward? Should we just commit the additional tests with this pull request?

[pelotom]

@oliviertassinari I was really hoping we'd find a way around the issue with inferring callback parameter types, but my bounty on the SO question hasn't yielded any new answers, so I'm losing hope :( I think if we could get it so that everything works as-is for the case where you're not overriding the component, and then if you do override the component you might need to provide more type information, e.g.

// works with no additional annotation:
<Avatar onClick={e => log(e)} alt="Image Alt" src="example.jpg" />
// requires annotating with either component or props type:
<Avatar<'button'> onClick={e => log(e)} component="button" alt="Image Alt" src="example.jpg" />
<Avatar<{ x: number }>
  component={(props: { x: number }) => <div />}
  x={3}
  // Should NOT be allowed:
  // onClick={e => log(e)}
  alt="Image Alt"
  src="example.jpg"
/>
// OR, requires giving parameter types for callbacks:
<Avatar onClick={(e: React.MouseEvent<HTMLButtonElement>) => log(e)} component="button" alt="Image Alt" src="example.jpg" />

then that would still be a huge win. This seems more feasible somehow, but I still haven't figured out a way to do it.

If the PR is viewed as clutter, we can close it, but I maintain hopes that it (or something like it) can be merged some day.

As far as merging the tests, some of them won't pass without the rest of the PR.

[oliviertassinari]

@pelotom Thanks for the follow up. Do as you see fit with this pull request.

[pelotom]

I've been playing around with a new strategy for solving this problem. It looks promising, but I've run into another TypeScript bug: microsoft/TypeScript#26004. Anyone interested in seeing this accomplished should go put their 👍 on it!

[nickserv]

Thanks for continuing to look into this, I'll track the upstream issues. This is still blocked by TypeScript, right? Let me know if I can do anything.

[pelotom]

This is still blocked by TypeScript, right?

@nickmccurdy yes, in particular by microsoft/TypeScript#26004 now.

[oliviertassinari]

I'm closing the pull request for now. Feel free to reopen it if any progress is made :). Thank you guys for the effort!

[kossmoboleat]

Seems like Microsoft closed the issue because it was fixed in a PR and that the new version 3.2.1 was also released:
microsoft/TypeScript#27408

[pelotom]

Thanks @kossmoboleat, I'll take another look at this soon!

[pelotom]

I've opened a new PR at #13868.