[typescript] Smarter logic?

[nenadalm]

Problem description

Button has href prop. If that prop is set, a element is used to render the button (https://material-ui-1dab0.firebaseapp.com/api/button/). It would be nice if Button had also target property.

Versions

• Material-UI: 1.0.0-beta.8

[oliviertassinari]

Any other properties supplied will be spread to the root element.

So you can provide a target property too.

[nenadalm]

@oliviertassinari I can't because I am using typescript and you have incorrect typings, so it complains that property target does not exists.

[oliviertassinari]

This is also linked to #8043

[oliviertassinari]

cc @benbayard

[mctep]

@benbayard I think this way should work for router links:

<Button component={props => <RouterLink {...props} to="/" />} />

@nenadalm and also for simple link:

<Button component={props => <a {...props} href="/" target="_blank" />} />

@oliviertassinari In TS typings component prop for Button should be React.ReactType not React.ReactNode

[sebald]

We had this "smarter logic" in the typings, but it didn't work out like we wanted it to :-/
You can read the reason why here: microsoft/TypeScript#17882

The only thing we can do is to allow all possiblities on the component. Meaning, allow all props from <a>.

[xaviergonz]

Talking about typings, ButtonProps (and probable others) for example miss style?: React.CSSProperties from its typing, and also some (all?) components miss stuff available to all react components such as tabIndex.

v0.xx types were ok in this regard.

PS: the original typings extend their props with extends React.HTMLAttributes<{}> to get this behavior right

[benbayard]

@mctep That looks fine to be, but it should be noted that inline lambdas are highly unperformant. If I were to write docs for this I would write it as:

function buttonLinkCreator(to: string) {
  return (props: ButtonProps) => <Link {...props} to={to} />
}

const HomePageLink = buttonLinkCreator('/');

class Blah extends React.Component {
  render() {
    return <Button component={HomePageLink} />;
  }
}

[sebald]

@xaviergonz Thanks for noting that. The new typings actually function the same way. If we missed one, please open up an issue! :) You already did, thanks!

@ALL I guess the easy fix would just be to make ButtonBase inherit from ButtonHTMLAttributes & AnchorHTMLAttributes. I am still hesitant to allow any props, which would mean adding the index signature to the ButtonBaseProps. If we do this, there is no protection against typos anymore.

What you all think?

[nenadalm]

Best would be to type it as good as typescript allows (so whitelist all possible allowed properties).

[xaviergonz]

What about using Pick from the react types we may need?
Pick is used like this:

interface Task {
 id: string,
 name: string,
 assignee: string,
 contacts: any[], //for brevity
 associatedJob: string,
 submissionDate: string,
 allocatedTime: number,
 expectedCompletion: string,
 invoiceNumber: string,
 invoiceDueDate: string,
 comment: string,
 taskAddress: string
 ...
}

type PartialTast = Pick<Task, 'id' | 'name' | 'contacts'> // an interface with only those properties from the original interface

so we could do something like

interface ButtonProps extends Pick<
  React.ButtonHTMLAttributes<HTMLButtonElement>, 
  'id' | 'className' | 'style' | 'title' | 'tabIndex' | 'disabled'
> {
  x: number;
  // whatever custom props including children
}

that way the types would be always in sync with react ones.

[benbayard]

@xaviergonz Why are we picking some properties out of button? Is it because there are some that are not supported. If so, you can use an Omit type which looks like this. It's much easier to maintain this way.

  /**
   * From https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-307871458
   * The Diff type is a subtraction operator for string literal types. It relies on:
   *  - T | never = T
   *  - T & never = never
   *  - An object with a string index signature can be indexed with any string.
   */
  export type StringDiff<T extends string, U extends string> = (
    {[K in T]: K} &
    {[K in U]: never} & {[K: string]: never}
  )[T];

  /**
   * From https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-311923766
   * Omits keys in K from object type T
   */
  export type ObjectOmit<T extends object, K extends keyof T> = Pick<T, StringDiff<keyof T, K>>;

[xaviergonz]

I didn't even know that was possible in TS oO

[sebald]

We're actually using this because some of the native APIs and the APIs of material-ui are sometimes not compatible 😉

https://github.com/callemall/material-ui/blob/1d84cb89a97755245d6b02921e983b39e4468204/src/Radio/RadioGroup.d.ts#L1-L11

---

But I think this discussion goes a little bit in the wrong direction. Why should we pick or omit things from the native elements.

@nenadalm wrote:

Best would be to type it as good as typescript allows (so whitelist all possible allowed properties).

This is actually why we didn't decide on that yet and the typings are (actually) kinda broken. IMHO the typings for ButtonBase should have the API provided by material-ui + <button> + <a>. If someone is (ab-)using the component prop and needs more/different API s/he can still to this, but loses type-safety (has to use as any). My guess is that the later will only be rarely used. Most people will use the components as a button or an anchor and we should primarily support this case and try to have as type-safe typings as possible, which we would have in my opinion we the aforementioned approach.

[xaviergonz]

I'd rather have the code compile and maybe get some runtime warnings from proptypes than having to use hacks like what I'm using right now for untyped properties, e.g:

<Button 
  color="primary"
  {...{
    title: "A button tooltip" // this works but is never type checked :(
  }}
>
  hi
</Button>

[benbayard]

@sebald I asked the same question, why are we picking any thing? (I'd prefer omiting over picking).

I think the component prop is perfect for creating a type-safe component until typescript can get bi-furcating types.

[sebald]

@xaviergonz With the proposed approach this would actually not be necessary, since title and a ton of other basic HTML attributes are covered by React.HTMLAttributes.

@benbayard Could you explain to me what you mean by "the component prop is perfect for creating a type-safe component"?

[sebald]

I made a PR with the mentioned approach, so when @oliviertassinari cuts a release on the weekend the typings will (hopefully) be better for everyone. If this is not enough, I'll add the index signature.

Notet that this will affect the following components:

• ButtonNavigationButton
• Butotn
• ButtonBase
• IconButotn
• ListItem
• TableSortLabel
• Tab
• Tabs
• TabScrollButton

[oliviertassinari]

@sebald Great! :)

[stunaz]

Hi, How would you type/fix this :

<ListItem
    component={Link}
    to="/about"
>

I have the following error:
file: 'file:///Users/stunaz/Sites/pegase-react-ts/src/modules/demo/Demo.tsx' severity: 'Error' message: 'Property 'to' does not exist on type 'IntrinsicAttributes & ListItemProps & { children?: ReactNode; }'.' at: '78,37' source: 'ts'

[stunaz]

@pelotom @sebald is the above supported ? if no is there a way around it?

[sebald]

You can always use spread and any as a workaround. This should work

<ListItem
    component={Link}
    {...{ to: '/about' } as any}
>

Though you will lose type safety.

---

I am not sure if to was ever support 😐 But I guess we could try to use tagged union types, to have better typings.

[stunaz]

to is a props used for the Link Component (react-router).
Currently with the ListItem I could pass as component={MyOwnComponent} and add additionals props destined for MyOwnComponent.

Ideally ListItem should accept his own props and props from the component'props.

The same goes for several others component as Grid, Card, Paper ...

[pelotom]

It might be worth considering adding

  button?: boolean;
  component?: React.ReactType;
+ additionalProps: {};
  dense?: boolean;
  disabled?: boolean;
  disableGutters?: boolean;
  divider?: boolean;

to ListItemProps and likewise all other components that allow overriding their root component. Then you could do

<ListItem
  component={Link}
  additionalProps={{ to: "/about" }}
/>

without being hampered by the type system. An escape hatch like this would allow us to avoid compromising the strict typing of the rest of the props, and just say, "if you're providing your own component you can throw whatever extra properties you like in here... use at your own risk!"

This will of course require a corresponding change to the .js implementations of all these components.

[pelotom]

Here's a less invasive and more type safe idea: define a HOC

declare const overrideComponent:
  <BaseProps extends { component?: React.ReactType}>(BaseComponent: React.ComponentType<BaseProps>) =>
  <CustomProps>(CustomComponent: React.ComponentType<CustomProps>) =>
  React.ComponentType<Replace<BaseProps, CustomProps>>

whose implementation is just

const overrideComponent = BaseComponent => CustomComponent => props =>
  <BaseComponent component={CustomComponent} {...props} />

and then you can do

const LinkListItem = overrideComponent(ListItem)(Link)

const elem = <LinkListItem to="/about" />

and it's completely type safe. On the decorated component you can provide all properties from the base component as well as the custom component, which matches the underlying semantics.

[oliviertassinari]

A better alternative where you will not risk a property collision:

<ListItem component={props => <Link {...props} to="/about" />}>
  // ...
</ListItem>

I have always considered the below pattern an elegant "hack" (it's harder to understand).
But what if both ListItem and Link implement a disabled property that you don't want to share the same value between? You use the pattern above, not below.

<ListItem component={Link} to="/about">
  // ...
</ListItem>

It's also why you won't find this third API with Material-UI. It's a tradeoff between the two previous ones that suffer limitations without being much simpler.

<ListItem component={<Link to="/about" />}>
  // ...
</ListItem>

[pelotom]

@oliviertassinari that's a great way to do it!

[stunaz]

just tested @oliviertassinari solution, worked fine, no more typings issue 👍
thanks all again

[activebiz]

This no longer works as per "@types/react": "^16.0.36",

[sebald]

If you're running into issues using Material-UI, please don't reply to old and closed issues. Open up a new one and provide as much info as possible. This way it's more likely we can help.