Render prop API?

[mbrowne]

There are missing props in the TypeScript type declaration file for ListItem, and also these types should ideally be generics.

• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

It should be possible to use the ContainerComponent and ContainerProps props of the ListItem component from TypeScript. Also, I expected that ListItem and MenuItem would be generic types to account for the fact that any additional props are passed to the component specified by the component prop, but they're not. To be specific, in this file, the interface is currently defined like this:

export interface MenuListProps ...

whereas I would have expected to see something like this:

export interface MenuListProps<TContainerComponentProps = {}> ...

Current Behavior

This TypeScript (.tsx) code does not compile:

<ListItem ContainerComponent="div" ContainerProps={{className: 'demo'}}>an item</ListItem>

Steps to Reproduce (for bugs)

Create the following test component in TypeScript:

import React from 'react'
import { ListItem, ListItemProps } from 'material-ui/List'

const Test: React.SFC<{}> = () => {
    return <ListItem ContainerComponent="div" ContainerProps={{className: 'demo'}}>an item</ListItem>
}
export default Test

Notes

The priority here is adding ContainerComponent and ContainerProps to the type declaration. Making it generic (e.g. MenuListProps<TContainerComponentProps = {}>) is arguably a best practice and would be nice, but might not be so practically useful in this case. If it were a class component it might be more helpful, but as far as I can tell, TypeScript does not provide a way to alias generic functions that would be any more concise than my current solution of using a typecast:

const MyMenuItem = MenuItem as React.ComponentType<MenuItemProps & MyComponentProps>
...
    render() {
        return <MyMenuItem component={MyComponent} somePropToMyComponent="demo">an item</MyMenuItem>
    }

Your Environment

Tech	Version
Material-UI	1.0.0-beta.32
React	16.2.0
browser	Chrome 64
TypeScript	2.7.1

[mbrowne]

Thanks! But does it make sense to limit ContainerProps to React.HTMLAttributes<HTMLDivElement> (and ContainerComponent to React.ReactType<React.HTMLAttributes<HTMLDivElement>>)? The default isn't even a 'div', it's 'li'. Maybe just make it React.HTMLAttributes<any>?

[oliviertassinari]

But does it make sense to limit

@mbrowne This is the simplest solution I can think of. @pelotom any idea around how we could improve the situation?

[mbrowne]

Maybe my suggestion of using a generic for the component prop would work for the ContainerComponent and ContainerProps props too...for example:

export interface ListItemProps<TComponentProps = React.HTMLAttributes<HTMLDivElement>, TContainerComponentProps = React.HTMLAttributes<HTMLLiElement>> ...

That way, the component and ContainerComponent props would default to prop types of HTMLDivElement and HTMLLiElement, respectively, but if the user wanted they could override the types so type-checking would work for additional props for some other component.

(BTW, in my initial bug report, I meant to refer to using a generic for MenuItemProps; I don't know why I wrote MenuListProps.)

[pelotom]

Maybe my suggestion of using a generic for the component prop would work for the ContainerComponent and ContainerProps props too

For generic props to be useful you also need generic components, and unfortunately those aren't well supported currently; see microsoft/TypeScript#6395.

I think this is fine, albeit not as type safe as one could hope:

ContainerComponent?: React.ReactType<React.HTMLAttributes<any>>;
ContainerProps?: React.HTMLAttributes<any>;

@oliviertassinari as a larger design note, the FooComponent/FooProps pattern that's common in Material UI presents a tricky problem for making type safe. It would be much easier and more powerful if we just used render props for these situations, e.g.

renderContainer?: (props: { className: string; children: ReactNode }) => ReactNode

Here you can be very specific about the props ({ className: string; children: ReactNode }) because you know that's exactly what will be passed, and it's up to the provider of the render prop to render that however they see fit. They can use whatever root element they want, whatever attributes they want, and even add more children. It allows complete customization while remaining perfectly type safe.

[oliviertassinari]

the FooComponent/FooProps pattern that's common in Material UI presents a tricky problem for making type safe. It would be much easier and more powerful if we just used render props for these situations

@pelotom I have added the render prop API as an investigation area before releasing v1. Right now, the main use case for the XXXProps is to simplify the property injection. People can always provide their own component but it's cumbersome. Is this change equivalent to just removing the ContainerProps property? There is a wide range of possible API combination. I'm not sure what's the best one.

[pelotom]

Is this change equivalent to just removing the ContainerProps property? There is a wide range of possible API combination. I'm not sure what's the best one.

The reason I suggested a render function instead of a component is that when you want to use an anonymous function to e.g. decorate with additional props,

<Foo
  ContainerComponent={({ className, children }) =>
    <Bar className={className} someAdditionalFlag />
      {children}
    </Bar>
  }
>
  ...
</Foo>

you explicitly don't want that function to be treated as a component, because it will be a new "component" on every render, and therefore Bar and everything below it will constantly get unmounted and remounted. That is, if what Foo renders is

<ContainerComponent className={className}>{children}</ContainerComponent>

then it's bad if ContainerComponent is an anonymous function. However if you had some logic to determine if ContainerComponent is actually just a simple function and in that case render

ContainerComponent({ className, children })

I think it would be fine.

[pelotom]

@oliviertassinari TL;DR I'm basically just recapping this twitter thread which you were a part of 😄
https://twitter.com/donavon/status/954788277416005632

[oliviertassinari]

you explicitly don't want that function to be treated as a component, because it will be a new "component" on every render

@pelotom Right. On the other hand, you can create an intermediary variable to hold the reference to your custom component, then you can add extra properties. I think that it's really about how we can make the API the simplest to play with.

const C = ({ className, children }) =>
    <Bar className={className} someAdditionalFlag />
      {children}
    </Bar>
 
<Foo ContainerComponent={C}>
  ...
</Foo>

[pelotom]

Right. On the other hand, you can create an intermediary variable to hold the reference with the CI.

That often requires you to do painful refactorings like turning this:

const MyComponent = props => {
  const x = ...;
  const y = ...;
  const z = ...;
  return (
    <Foo
      ContainerComponent={({ className, children }) => (
        <Bar className={className} x={x} y={y} z={z} />
          {children}
        </Bar>
      )}
    >
      x={x}, y={y}, z={z}
    </Foo>
  )
}

into this:

class MyComponent {

  ContainerComponent = ({ className, children }) => (
    <Bar className={className} x={this.x} y={this.y} z={this.z} />
      {children}
    </Bar>
  )

  render() {
    return (
      <Foo ContainerComponent={this.ContainerComponent}>
        x={this.x}, y={this.y}, z={this.z}
      </Foo>
    )
  }

  get x() {
    return ...;
  }

  get y() {
    return ...;
  }

  get z() {
    return ...;
  }
}

Not to mention that most people won't even be aware they need to do that refactoring, and will be confused when their app is extremely inefficient.

I think that it's really about how we can make the API the simplest to play with.

Agreed, I think the simplest and friendliest thing for users would be to take a component prop but inline SFCs when rendering.

[pelotom]

const C = ({ className, children }) =>
    <Bar className={className} someAdditionalFlag />
      {children}
    </Bar>
 
<Foo ContainerComponent={C}>
  ...
</Foo>

If you just extract it to a variable in the render method, you haven't saved anything; it will still be a new component on every render. And if someAdditionalFlag is something that had to be computed from the props, you can't define C at the module level. You're forced to make your component into a class if it wasn't already, with C a field on it (see example above).

[oliviertassinari]

@pelotom I agree, I definitely see value in the render prop API. My concern is much more about the execution. Who do you think we should move forward?

Not to mention that most people won't even be aware they need to do that refactoring, and will be confused when their app is extremely inefficient.

It's a good point.