search

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.08k stars 32.05k forks source link

[RFC] What's the best API to override deep/nested elements? #21453

Closed oliviertassinari closed 3 years ago

oliviertassinari commented 4 years ago

A while back, around the alpha & beta iterations on the v1 version, we had to decide what was the best way to override the nested components. It's meant to help developers customize the rendering. We can find #11204 and #10476 as a legacy.

up until v1

So far, the tradeoff on this problem has been:

  1. Provide a XXXComponent prop when overriding the nested element can be valuable for users. For instance:

https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui/src/Modal/Modal.d.ts#L8

  1. Provide a XXXProps prop when providing a custom XXXComponent component is too cumbersome. For instance:

https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui/src/Modal/Modal.d.ts#L9

However, this original design constraint is increasingly more challenged by the following:

Autocomplete

The Autocomplete mixes the renderXXX approach with the XXXComponent approach. cc @oliviertassinari

https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts#L164 https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts#L142

Date Picker

The DatePicker mixes the renderXXX approach with the XXXComponent approach. cc @dmtrKovalenko

renderLoading?: () => React.ReactNode;

https://github.com/mui-org/material-ui-pickers/blob/c834f5cac5930df099aaad806ceb3fe4ec1669db/lib/src/views/Calendar/Calendar.tsx#L43

ToolbarComponent?: React.ComponentType<ToolbarComponentProps>;

https://github.com/mui-org/material-ui-pickers/blob/c834f5cac5930df099aaad806ceb3fe4ec1669db/lib/src/typings/BasePicker.tsx#L61

Data Drid

The DataGrid goes with xXXComponent approach, however, the name is confusing. Sometimes it's a render prop, sometimes it's a React element, it's never a component as the name suggests cc @dtassone

  paginationComponent?: (props: PaginationProps) => React.ReactNode;
  loadingOverlayComponent?: React.ReactNode;
  noRowsOverlayComponent?: React.ReactNode;
  footerComponent?: (params: ComponentParams) => React.ReactNode;
  headerComponent?: (params: ComponentParams) => React.ReactNode;

https://github.com/mui-org/material-ui-x/blob/59d533642d5837ce6912fe88cbcdfc228f621594/packages/grid/x-grid-modules/src/models/gridOptions.tsx#L93-L97

Styled components

Styled components might request something brand new. cc @mnajdova As the experimentation of #21104 showcases. If we want to keep the CSS specificity at it's lowest possible level (meaning one level) and expose unstyled components, we will have to expose an API to inject custom component. In older experimentation, I worked around the problem with a components prop.

components?: {
  Root: React.ElementType<React.HTMLAttributes<HTMLDivElement>>;
  Label: React.ElementType<React.HTMLAttributes<HTMLSpanElement>>;
}

https://github.com/oliviertassinari/material-ui/blob/153c1833fb204a28ee6712db1c2ac6a9308a163e/packages/material-ui/src/TableCell/TableCell.unstyled.js#L18

The problem

I think that it would be great to defines which API works best and in which cases. I would hope that by doing so, we can provide a consistent experience for the developers using the library to build applications and websites. I also think that by reducing the number of approaches we can reduce the learning curve for new users.

When should we use a component, when should we use a render prop, etc.?

Prior-arts

oliviertassinari commented 4 years ago

While in my initial comment was trying to draw an accurate presentation of what we have explored so far, I'm going to try to explore the pros & cons of each approach we have learned.

Component API

Historically, we went with the API because it was minimizing the complexity of the source. It was requiring less boilerplate to implement. It was making the source easier to read. I suspect there are a lot of people looking at the source, either for learning best practices, how to apply customization, to debug something, to contribute, etc.

The second advantage is that it allows using the hook API directly.

render prop API

However, we have quickly realized the limitations of the component API. The limitation is twofold:

  1. Some people create a new component in the render method, which leads to a remount of DOM nodes at each render. While the issue might hit you silently, it harms performance and is very noticeable when it impacts the transition: it breaks them.
  2. Now, if we look at why people create new components in the render method. It's because either they don't know well the distinction between a component, an element, and a function in the React landscape, or because they need to access a variable from outside the component scope.

I think that it's this latter reason that leads us to introduce more render like APIs lately. Without such API, you have two alternatives, that might be cumbersome:

Multiple props vs one prop

I'm switching gears a little bit here on a different concern: scalability. Should we have one prop per customization point or should we group the customization points under a single prop? So far, we have had different answer to this problem:

mnajdova commented 4 years ago

Sharing my experience and thoughts on this matter:

as/component prop

Having a property like as/component on each component, where you can specify whatever component you want to be render as root. For example you can do the following:

<Button as="div">Hello</Button> - and the root will not be the default button, but div

renderXXX props

But then clients want to customize the different things you have inside the button, like the icon or the content... Historically in Fluent UI we had a render props, for example if I want to render the text inside the Button with Typography from Material-UI, I could provide something like:

<Button renderContent={(Component, props) => <Typography {...props}>Hello</Typography>} />

But this created the props explosion, with each "slot" property we would have to have appropriate render property. In addition it is confusing what will happen if you provide both (we were sending as an argument the shorthand's (content) props in the renderContent callback, but it was still very confusing)

<Button content="Hello" renderContent={(Component, props) => <Typography {...props}>{props.text}</Typography>} />

children callback

For solving this, we decided to support children callback on all shorthand props, where people can customize what is rendered there:

<Button content={{ text: 'Hello', children: (C, p) => <Typography {...p}>{p.text}</Typography>}} />

createComponent/compose utility

This works great, but if you want in your application to have everywhere Typography you don't want to do this in all component instances, so you could theoretically create wrapper that will handle it, but we thought we can provide better API for this - createComponent/compose:

const CustomButton = createComponent(Button, { 
  slots: { 
    content: Typography 
  } 
}):

The beauty of an API like this is, you can create component for each of your slots, for example using styled components, with which you can theoretically opt-out of the styling mechanism that is used otherwise.

dtassone commented 4 years ago

I think we can split the problem in 3 categories

  1. Static/Dumb component swap, ie replace Icon sort in the grid
  2. Dynamic or using parent component props, ie pagination in the grid. How do we pass props between the parent components and its customised child? Render props, hooks, context, api... Do we have a standard?
  3. Tree structure, that mixes the 2 categories above, ie the footer component that includes a pagination component
dtassone commented 4 years ago

FYI I have refactored the approach on the grid for the footer and header, which were the only ones using children. All components customisation available here if you want to check it out ;)

oliviertassinari commented 4 years ago

@dtassone Thanks for sharing your exploration, once we settle on one approach we can update all the components.

Proposal number 1

components?: {
  Root?: React.ElementType; // equivalent to classes.root
  Label?: React.ElementType; // equivalent to classes.label
};

This also means moving the existing XxxComponent props under this new umbrella. This same API will be exposed to the future unstyled components, allowing full customizability with styled components. I don't have any strong point of view on the name, we could call it slots.

cc @mui-org/core-team

yordis commented 4 years ago

I definitely support moving to this:

// notice slots
slots?: {
  // notice the functions
  Root?: (props)=> React.ElementType; // equivalent to classes.root
  Label?: (props)=> React.ElementType; // equivalent to classes.label
};

Matches classes and it is easier to understand what the new slots scoped prop is used for across Mui. That being said, I much rather use a function that returns the component. That will allow me to create static components, and deal with variable hoisting in the factory function rather than in the components.

I have documented this pattern for our internal React components in https://github.com/straw-hat-team/adr/blob/master/adrs/1358452048/README.md.

ryancogswell commented 4 years ago

That being said, I much rather use a function that returns the component.

@yordis Could you provide an example to demonstrate the benefit of a function here rather than just React.ElementType? It isn't clear to me how you would picture this working or what the benefit is, and it seems more complicated to use.

With the following:

components?: {
  Root?: React.ElementType; // equivalent to classes.root
  Label?: React.ElementType; // equivalent to classes.label
};

users can do things like:

const StyledDiv = styled.div`
  background-color: blue;
`;
...
components: {
  Root: StyledDiv 
};

It seems cumbersome and confusing to instead do 

components: {
  Root: () => StyledDiv 
};
yordis commented 4 years ago

@ryancogswell hey there, let me know if the following example makes sense.

I think your example is valid, but there are some caveats to it since that example is for trivial cases, sometimes you will need to remap or access hoisted values so you can't simply do that.

Thinking out loud, the differences between a Function and React Component is technically none for the end-users, except for Mui internals and React itself, so I guess you can take a "Component" rather than a function anyway.

import ExternalComponent from 'whatever-thing-i-dont-control';

// static, it doesn't require runtime.
const SomethingNoHoisted = (props)=> {
  return <ExternalComponent remapped={props.label} title={props.title}/>
}

function MyComponent(props) {
  const title = props.title ?? 'Unknown';

  // I guess you co do this, the only thing you are missing is that Mui
  // will use `createElement` or <Something/> over Something() in the internals (or you change to createSomething), so no a big
  // deal I guess.

  // The caveat is that you lose some static declaration of the component
  // and require runtime for it, meh.

  // So personally speaking, I don't see these functions as component but factories.
  function Something(props) {
    return (
      // Has access to anything internal from MuiComponent
      // passing using props in case that some computations from inside
      /// needs to be exposed to the outside scope
      <ExternalComponent remapped={props.label}
        // Remapping in case you need it
        remapped={props.label}
        // Access to hoisted variables,
        title={title}/>
      )
  }

  return (
    <div>
      <MuiComponent
        slots={{
          // Something: Something,

          // I can't do this due to prop hoisting values, and props that needs
          // to be remapped.
          // Something: ExternalComponent

          // I don't control the rendering of the component, therefore, I can't
          // pass `title` from the hoisted variable.
          // Something: SomethingNoHoisted
        }}    
      />
    </div>
  )
}

And I can't agree that it is cumbersome and confusing due to my personal experience, I can only speak from my perspective.

And probably you are right, I am thinking in FP practices for function composition that we don't need in some cases and will create harder to understand code for some people.

I take back my suggestion, doesn't matter to me.

ryancogswell commented 4 years ago

@yordis It seems that the main problem you are trying to solve (or at least a different way to frame the problem) is how to pass additional props to your custom component. One way this has been solved in Material-UI is a corresponding XXXProps prop.

For instance, Modal (in Olivier's intial comment) has a BackdropProps prop in addition to a BackdropComponent prop. With the new structure, it might look more like:

<Modal components={Backdrop: CustomBackdropComponent} BackdropProps: { customProp1: props.valueFromParentScope } ...>...</Modal>

or if we take the complicated case in your example (the one requiring a function) it could look like:

import ExternalComponent from 'whatever-thing-i-dont-control';

// This can now still be static.
function Something({label, title, ...other}) {
    return (
      <ExternalComponent remapped={label} title={title} {...other}/>
      );
}

function MyComponent(props) {
  const title = props.title ?? 'Unknown';
  return (
    <div>
      <MuiComponent components={{ SomeMuiNestedComponent: Something}} SomeMuiNestedComponentProps={{ title }} />
    </div>
  )
}

In this scenario MUI would merge the props specified via SomeMuiNestedComponentProps with the props that it would send to SomeMuiNestedComponent.

As a side note, I prefer the name components over slots since I think it more clearly communicates the type expected (assuming the type is React.ElementType) and retains more consistency with prior Material-UI naming conventions.

yordis commented 4 years ago 
<Modal components={Backdrop: CustomBackdropComponent} BackdropProps: { customProp1: props.valueFromParentScope } ...>...</Modal>

That example is exactly what I am trying to prevent since I been there. This is why either to use a factory function which I personally prefer or a dynamic component in the scope. I have been bitten by that example multiple times and the problems that bring in the future.

Reasons

  1. It definitely increases the error-prune by mismatching
  2. Computations are wasted in cases where the SomeMuiNestedComponent component is not being rendered, but you computed all the properties for it regardless (nested loops doing these practices, the refactoring was a nightmare to track since happened at multiple levels).
  3. Mui doesn't need to take such complexity to make sure to proxy the right props to the right component at the right time.
  4. The functional style is friendly to test environments since you can always isolate the hoisting out of the component test, potentially testing the data transformation in isolation if needed and compose more functions to create the component factory function (meh).
oliviertassinari commented 4 years ago

@yordis To take your example, using props from the outside would look like this, with the first proposal. We would use the context:

import ExternalComponent from 'whatever-thing-i-dont-control';

const Context = React.createContext();

function Something(props) {
  const { title } = React.useContext(Context);

  return (
    <ExternalComponent remapped={props.label}
      remapped={props.label}
      title={title} />
  )
}

function MyComponent(props) {
  const title = props.title ?? 'Unknown';

  return (
    <div>
      <Context.Provider value={{ title }}>
        <MuiComponent
          components={{
            Something,
          }}
        />
      </Context.Provider>
    </div>
  )
}

It comes with two drawbacks compared to the XxxProps props: 1. It forces you to import the correct element for the slot, you have to find it (can probably be solved with great conventions and documentation), 2. It's more boilerplate.

2. A second possible proposal

The same one as the first, however, we introduce a prop to match the current use cases for the XxxProps props.

components?: {
  Root?: React.ElementType<RootProps>;
  Label?: React.ElementType<LabelProp>;
};
componentsProps?: {
   root?: RootProps;
   label?: LabelProp;
};

This option has the advantage of allowing to customize nested components without even needing to import them.

3. A third possible proposal

This time with render props, all in. For the cases where the render prop is mandatory, like renderInput we would expose it independently.

renders?: {
  root?: (props: RootProps) => React.ReactNode;
  label?: (props: LabelProp) => React.ReactNode; 
};

I see two downsides with this approach

  1. The developers will need to import the correct components. This can be mitigated with awesome documentation and great conventions.
  2. The source of our components will look "strange", we will no longer be able to use the JSX syntax. Imagine we were only using the React.createElement syntax, it would look like the same. It might be disorienting for the developers looking at our source to figure customizability issue or inspiration.
const Button = React.forwardRef(function Button(props, ref) {
  const {
    children,
    classes,
    className,
    color = 'default',
    component = 'button',
    renders = { root = props => <ButtonBase {...props} />, label: props => <span {...props} /> },
    disabled = false,
    disableElevation = false,
    disableFocusRipple = false,
    endIcon: endIconProp,
    focusVisibleClassName,
    fullWidth = false,
    size = 'medium',
    startIcon: startIconProp,
    type = 'button',
    variant = 'text',
    ...other
  } = props;

  const startIcon = startIconProp && (
    <span className={clsx(classes.startIcon, classes[`iconSize${capitalize(size)}`])}>
      {startIconProp}
    </span>
  );
  const endIcon = endIconProp && (
    <span className={clsx(classes.endIcon, classes[`iconSize${capitalize(size)}`])}>
      {endIconProp}
    </span>
  );

  return renders.root({
    className: clsx(
      classes.root,
      classes[variant],
      {
        [classes[`${variant}${capitalize(color)}`]]: color !== 'default' && color !== 'inherit',
        [classes[`${variant}Size${capitalize(size)}`]]: size !== 'medium',
        [classes[`size${capitalize(size)}`]]: size !== 'medium',
        [classes.disableElevation]: disableElevation,
        [classes.disabled]: disabled,
        [classes.fullWidth]: fullWidth,
        [classes.colorInherit]: color === 'inherit',
      },
      className,
    ),
    component,
    disabled: true,
    focusRipple: !disableFocusRipple,
    focusVisibleClassName: clsx(classes.focusVisible, focusVisibleClassName),
    ref,
    type,
    ...other,
    children: render.span({
      className: classes.label,
    })
  })
});

On a related note, during my exploration of the unstyled story, I ended up trying a prop to solve a similar problem:

forwardProps?: (slot: 'root' | 'label', state) => props;

The prop (or something similar) is required to give the nested styled-components access to the external props and internal state of the component, to style it accordingly. It also assumes that we want to keep the CSS specificity at 1.

dmtrKovalenko commented 4 years ago

My personal opinion:

How to store the props?

Here we have only 2 possible options: flat vs deep. I believe that flat option is more useful because it is

Components vs functions

Personally I have no preference on that. There is no performance difference even on the big amount of nodes.

Also could say that render functions look more expressive from the reading side. In this example, if you are looking for overrides rendering you could miss the components prop because it is just an object

<Something
  SelectComponent={Select}
/>

And here your editor will shout you that something is rendering over here. 

<Something
  renderSelect={props => <Select {...props} />} 
/>
yordis commented 4 years ago

Reading your examples and opinions, and based on my experience, I definitely will stick to factory/render function to tackle this problem.

Something else we could do is to adopt some practices from the Vue community if I am not mistaken they use factory/render functions to tackle this issue.

I like that they use slots (close to what the HTML spec suppose to do) and the naming is normally a noun instead of a verb (label vs renderLabel). I wouldn't mind align with them in this regard, it looks clean personally speaking, and it tackles all the technical use cases so far.

// some generics
interface MuiUniversalProps <T = unknown> {
  slots?: T;
}

interface Slot<T = unknown> {
  (props: T): React.Child;
}
// button component
interface ButtonSlots {
  root: Slot<{}>;
  label: Slot<{}>;
}

interface ButtonProps extends MuiUniversalProps<ButtonSlots> {
  ...
}

I don't think I have much to contribute at this point, I trust your judgment.

oliviertassinari commented 4 years ago

flat vs. deep

@dmtrKovalenko While I think that we should encourage flatten props as much as possible for the reasons mentioned in https://github.com/mui-org/material-ui/issues/21453#issuecomment-656065074. (@dtassone Yes, I very much have the DataGrid API in mind the options could be flattened :p), It's not without its limitations.

I see a couple of advantages in going deep:

dtassone commented 4 years ago

Thanks @oliviertassinari :)

I think both ways are very discussable if we go deep we have clean and tidy props such as, options, events components... However, properties like loading which just turn the loading overlay for the grid, or disabled don't really belong in options. I mean they could be there. But I find it unpractical to put them in a nested object. Could we go for an hybrid approach, should we flatten state boolean props, or should we just flatten everything?

If we decide to flatten the props, then, inside the component, we will probably have to restructure some of the props together, so we can process or observe them together.

Another kind of drawback is that we mind end up with a very long list of props for some components and it might be difficult to find what we want in the middle of everything.

Events, components are quite limited, but some components might have a long list of configuration options. So should we consider to flatten everything except configuration options...

oliviertassinari commented 4 years ago

I would vote for the proposal 2. The 3rd one seems to be a no-go regarding the readability of the source at scale. Readability is still OK for one-off cases. The 1st one is quite limiting for leveraging props from the outside. it's cumbersome to leverage the context. Hence the second as a tradeoff. It's basically the 1st proposal but extended with components and componentsProps).

dimitropoulos commented 4 years ago

Has anyone taken a look at an approach like React Aria is using?:

import {useButton} from '@react-aria/button';

function Button(props) {
  let ref = React.useRef();
  let {buttonProps} = useButton(props, ref);

  return (
    <button {...buttonProps} ref={ref}>
      {props.children}
    </button>
  );
}

<Button onPress={() => alert('Button pressed!')}>Press me</Button>

I've used a couple APIs like this (another is react-swipeable). Perhaps, irrespective of whatever deeper-level abstraction is picked from what's being discussed above, material-ui can incorporate an API that sort of "handles the details" for you, by using this technique.

That way, if a user wants to override something, they can dig in and find out more about the structure, but if they don't then they don't have to. React Aria's API is like this in most places, so take a look there for more concrete examples of it working in practice.

oliviertassinari commented 4 years ago

@dimitropoulos This RFC doesn't aim to solve the whole problem of customizability. There are different levels of abstraction that are viable, each with pros & cons, I would put them in 3 buckets:

  1. A hook first approach. This gives maximum flexibility, great for power users that understand the value of a11y. e.g. https://www.downshift-js.com/.
  2. A component first approach with components close to the DOM nodes. This is great for composition and can be more practical for users than hooks. e.g. https://reach.tech/combobox.
  3. A high-level component approach. Sometimes, they would be too many components to compose to make it feasible. Imagine react-table scaling to the needs of ag-grid: no way. e.g. https://react-select.com/.

The RFC focuses on improving 3. This is a problem we face with the upcoming DataGrid and DatePicker components. The RFC is hopefully also something I hope we can use to provide unstyled components or different styles engine.

For 1. it will require a different effort. But I definitely agree that it would make sense to unbundled the component we have, like the angular CDK. Do you have any interest in helping us with this? For 2. it's the approach we have tried to follow (but with exceptions).

dimitropoulos commented 4 years ago

sure sure, yeah, I get that, I was just saying that whatever solution this RFC lands on - it would be cool to keep the React Aria-style API above in mind because it offers a lot of flexibility which I (at least tangentially) view to be a deeper goal of this RFC: so I thought I'd mention it.

oliviertassinari commented 3 years ago

An update, proposal 2 has been implemented with the Slider, Badge, and recently the DatePicker. We are pushing further with this approach.

dimitropoulos commented 3 years ago

@oliviertassinari and, to confirm, part of the design (not just happy accident) with proposal 2 is that you can pass componentProps to something you didn't necessarily pass components for, right?

<MuiXXX
  components={{
    Root: SomeRootComponent,
    Label: SomeLabelComponent,
  }}
  componentsProps={{
    root: { someProp: 'some value' },
    label: { someProp: 'some value' },
  }}
/>
oliviertassinari commented 3 years ago

Closing as we agreed on:

<MuiXXX
  components={{
    Root: SomeRootComponent,
    Label: SomeLabelComponent,
  }}
  componentsProps={{
    root: { someProp: 'some value' },
    label: { someProp: 'some value' },
  }}
/>

going forward.