Slots: Refactor and add options structure for more rendering options.

[JasonGore]

Pull request checklist

• Addresses an existing issue: Fixes Foundation: 'as' Props #8334, Foundation: Render Functions #8335
• Include a change request file using $ npm run change

Description of changes

• Add new options structure ISlotOptions, giving slots a powerful mechanism for adding features over time without breaking backwards compatibility.
• Reduce friction around scenarios where users want to provide both props and implementation to a given slot.
• Add new simplified render function that still allows implementations to wrap default content without the added render argument.
• Add new component option that allows a user to simply provide a component implementation taking in the slot props.
• Remove the more complex async render function until specific use cases warranting its use are identified.
• Change the fundamental ISlotProp signature. It is now a union of shorthand (string, number or boolean) and ISlotOptions, shown here consolidated and simplified:

export type ISlotProp<TProps, TShorthandProp> = TShorthandProp | ISlotOptions<TProps>;

export interface ISlotOptions<TProps> {
  /**
   * Props object.
   */
  props?: TProps;
  /**
   * Stateless component implementation. Mutually exclusively with render.
   */
  component?: React.ReactType<TProps>;
  /**
   * Render implementation. Mutually exclusively with component.
   */
  render?: (props: IPropsWithChildren<TProps>, defaultComponent: React.ComponentType<TProps>) => JSX.Element;
}

Example Improvements

To do a customized CollapsibleSection with tooltip and icon such as this:

Before:

Previously required a render function and a closure just to capture user props:

const customizedTitle = (text: string): ICollapsibleSectionProps['title'] =>
  render =>
    render(
      (DefaultContent, props) => (
        <CustomizedCollapsibleSectionTitle {...props} />
      ),
      text
    );

<CollapsibleSection title={customizedTitle('Persona')}>
  Content 1
</CollapsibleSection>

After:

Now can be done with just a props object and component:

<CollapsibleSection title={{ props: { text: 'Persona' }, component: CustomizedCollapsibleSectionTitle }}>
  Content 1
</CollapsibleSection>

Before:

And even simpler scenarios where just the component implementation is changed:

<Stack
  // Render the inner content as a ul (there's not currently a less-verbose way to do this)
  inner={render => render((_, props) => <ul className={props.className}>{props.children}</ul>)}
/>

After:

Now can just use component and automatically have props applied.

<Stack inner={{ component: 'ul' }} />

And render functions are now also easier to use, while still allowing the capability to wrap default render content:

Before:

<Button
  icon={render =>
    render((IconType, iconProps) => (
      <b>
        Icon: <IconType {...iconProps} iconName="upload" />
      </b>
    ))
  }
  content="Icon: Function, Text + Icon"
/>

After:

<Button
  icon={{
    render: (iconProps, DefaultComponent) => (
      <b>
        Icon: <DefaultComponent {...iconProps} iconName="upload" />
      </b>
    )
  }}
  content="Icon: Function, Text + Icon"
/>

I found multiple TS issues while doing this PR that unfortunately have impacted this PR. These are items impacted:

• Loss of type safety inside props definitions as well as props use in component and render where excess (mistyped) prop names do not error.
• Can't provide extra constraints on TProps used with ISlotProps (currently allows functions)
• Can't allow shorthand in props for ease of use, which would be beneficial primarily when component and render are also provided.
• Unused variable workarounds in tests.

Related TS PRs / issues:

• Fix nested excess property checks when target is a union with non-discriminated + non-object constituents TypeScript#30771
• Perform excess property checking on intersection and union members TypeScript#30853
• Negated types TypeScript#29317
• Support @ts-ignore for specific errors TypeScript#19139

Focus areas to test

Add unit tests and typing tests for Foundation.

Microsoft Reviewers: Open in CodeFlow

[msft-github-bot]

Component perf results:

Scenario	Target branch avg total (ms)	PR avg total (ms)	Target branch avg per item (ms)	PR avg per item (ms)
PrimaryButton	101.878	102.544	1.019	1.025
BaseButton	34.725	36.399	0.347	0.364
NewButton	115.509	116.907	1.155	1.169
button	5.562	5.238	0.056	0.052
DetailsRows without styles	188.399	188.697	1.884	1.887
DetailsRows	199.655	206.987	1.996	2.070
Toggles	52.057	52.600	0.520	0.526
NewToggle	72.553	69.629	0.726	0.696

[JasonGore]

@khmakoto Curious what you think of this. menu is provided in the top level props. Having it required in the props also requires it at the slot level any time any props are provided for the slot. I'm not sure best how to solve this yet, but I'm wondering if having menu be optional is one solution.

[khmakoto]

Yeah, I think my idea originally was to have menu be required for the MenuButton given that we were separating it from the regular Button. But now that we're using MenuButton as a building block for other components (and which is the real reason to use foundation in the end) then I think having menu be optional is a good solution. We should just check that everything still works as intended.

[JasonGore]

Do you want to enter an item for this or is the change marked todo? Thanks!

[khmakoto]

I've marked it as a to-do in #6134.

[khmakoto]

Is there any work left to do on this PR? If not, I can approve it, but I want to make sure I don't approve it prematurely.

[natalieethell]

The Text and Announced example changes look good

[khmakoto]

LGTM 👍

[lijunle]

Hi, all.

Passing an object to React element will break the shallow compare for optimization. For the { render } prop, why not directly flatten the render props as one prop?

Existing:

<Button
  icon={{
    render: (iconProps, DefaultComponent) => (
      <b>
        Icon: <DefaultComponent {...iconProps} iconName="upload" />
      </b>
    )
  }}
  content="Icon: Function, Text + Icon"
/>

Why not?

// Declare in a static place to avoid re-create on each run.
function renderIcon(iconProps, DefaultComponent): JSX.Element (
  <b>
    Icon: <DefaultComponent {...iconProps} iconName="upload" />
  </b>
)

// Pass the shallow compare check.
<Button
  renderIcon={renderIcon}
  content="Icon: Function, Text + Icon"
/>

[JasonGore]

Slots props objects are not passed directly into React.createElement. This slot object is evaluated and broken out internally. The props property here is what is passed to React.createElement, not the whole parent object.

[JasonGore]

We've also decided to consolidate all of these types of properties (render function, component implementation, props objects, and shorthand) because traditionally we have flattened props, which has caused prop interface explosion and inconsistent naming and implementations across our component library.

[lijunle]

Well, the nest props are breaking the shallow comparison on the first level optimization.

Give an example, I have a component named App. Each input change event trigger the App component re-render. Then, the {props:{iconName:'Submit'}} is recreated each time on render run, so the shallow compare on IconButton will not work.

function App() {
  const [inputValue, setInputValue] = React.useState('');
  return (
    <fomr>
      <input value={inputValue} onChange={setInputValue} />
      <FabricReact.IconButton
        icon={{ props: { iconName: 'Submit' } }}
        label='Submit'
      />
    <form>
  );
}

---

If we use something like this, the IconButton will optimize the continuous render because the reference of renderIcon function is always the same.

function renderIcon(DefaultIcon, defaultProps): JSX {
    return (<DefaultIcon {...defaultProps} iconName='Submit' />);
}
function App() {
  const [inputValue, setInputValue] = React.useState('');
  return (
    <fomr>
      <input value={inputValue} onChange={setInputValue} />
      <FabricReact.IconButton
        renderIcon={renderIcon}
        label='Submit'
      />
    <form>
  );
}

[dzearing]

Correct me if I'm wrong, but I believe this only matters when:

1. The component is pure.
2. The component renders no children, or explicitly ignores child mutations.
3. The caller doesn't inline functions (which seems to be what a lot of React hooks tutorials are suggesting to do.)

Example here: https://codesandbox.io/s/pm6zy9z400

Props don't mutate, but just by having children, it makes the "pure" or "memo" useless.

So at least from my perspective for pure to work well, where you actually avoid re-rendering as a result of props not changing, you need the rules above (especially 2) to apply.

So something like Icon would be a great pure candidate, while Button, which could have child elements, would not. Or, we don't allow children and only allow props. Thoughts?

[JasonGore]

If you want to bake the iconName value of Submit inside of your render function, you can do that with slots too. There is nothing that says render for slots can't be a constant reference, either. props is only provided for situations where you want dynamically changing props values.

The slots equivalent is more similar to your optimized example:

const iconSlot = {
  render: (DefaultIcon, defaultProps): JSX => {
     return (<DefaultIcon {...defaultProps} iconName='Submit' />);
 }
};

function App() {
  const [inputValue, setInputValue] = React.useState('');
  return (
    <form>
      <input value={inputValue} onChange={setInputValue} />
      <FabricReact.IconButton
        icon={iconSlot}
        label='Submit'
      />
    <form>
  );
}

[dzearing]

What @JasonGore said should avoid rerenders, assuming IconButton is pure.

Another thing we've been discussing is that the createComponent helper could extend existing components. That means that if you wanted to provide a custom icon, you could use it to easily create a new component which memoizes your slot defaults.

Pure components are certainly fragile and easy to mess up on, and I hear the concern that any complex object prop type, or functional prop type, or even children, could cause pure components to always rerender.

[lijunle]

@dzearing @JasonGore I think all of your points are correct.

I think the core problem is the balance of the abstraction and customization. Take contextual menu as an example. Here is one screenshot of the contextual menu:

Think about how it is created from JSX tree. It may be something like this:

function MyContextualMenu(): JSX.Element {
  return (
    <>
      <ToggleButton text="Click for ContextualMenu" />
      <Popup>
        <MenuItem text="New" />
        <MenuItemBar />
        <MenuItem text="Rename" />
        <MenuItem text="Edit" />
        <MenuItem text="Properties" />
        <MenuItem text="Link same window" />
      </Popup>
    </>
  );
}

Then, we want make a component named ContextualMenu to abstract the JSX tree construction. So, we need to design a props interface like this:

interface IContextualMenuProps {
  text: string;
  menuItems: { text: string; key: React.Key; }[];
}

Compare the props interface with the JSX tree, they looks very similar. Although we want to make an abstraction layer, but it actually does not work.

Worse, if we are supporting more scenario like, submenu or icon for menu item. We are extending the interface props just as the JSX tree.

interface IContextualMenuItem {
  key: React.Key;
  text: string;
  icon?: string;
  children: IConextualMenuItem[];
}
interface IContextualMenuProps {
  text: string;
  menuItems: IContextualMenuItem[];
}

If I really understand why the props is structured like this, I know how the JSX tree is rendered. So, there are choices - do you want to write this huge complicated props object, or a controlled JSX tree?

The complexity of the component leads the complexity of the component props interface. This is exactly one case of leaky abstraction. It is not a fault of pure component, it is solving the problem to configure everything in JSX tree from props.

[JasonGore]

David and I were talking about this quite a bit. The line between "what to render" and "how to render" is not black and white, particularly when you consider props like styles.

I hope we all agree that mixing in "how to render" implementations is unavoidable: there is a lot of demand for render functions and replaceable subcomponents. As a result, I think this is primarily a matter of what the props interface looks like.

I've been thinking of a couple of ideas and have prototyped one already.

One option is to limit "how" to component creation (bind time) so that it doesn't mix in with render functions, but I worry that's a bit too limiting.

Another option is to separate out the "how" from the "what" props with a new slots prop (setting aside the use of inline objects):

const iconSlot = {
  render: (DefaultIcon, defaultProps): JSX => {
     return (<DefaultIcon {...defaultProps} />);
 }
};

function App() {
  const [inputValue, setInputValue] = React.useState('');
  return (
    <form>
      <input value={inputValue} onChange={setInputValue} />
      <FabricReact.IconButton
        icon={{ iconName: 'Submit' }}  // "what" to render
        label='Submit'
        slots={{ icon: iconSlot }}  // "how" to render it
      />
    <form>
  );
}

What do you think?

[lijunle]

I hope we all agree that mixing in "how to render" implementations is unavoidable: there is a lot of demand for render functions and replaceable subcomponents.

I totally agree. We are kinds of exposing the details about how to render - that is unavoidable. I don't think we can/should solve the problem to hide everything inside one component.

As a result, I think this is primarily a matter of what the props interface looks like.

I could think from another angle. First, there are always different requirements on one component - w/o icon, w/o image, different font-size, margin/padding, etc. I think we should solve the problem about what if the one-stop component cannot fit the needs, what is the easiest way to let consumers quickly unblock themselves.

What is the current answer for this problem? The consumer needs to:

1. Introduce a new prop in fabric-react component
2. Wait for fabric-react bump up
3. Upgrade the fabric-react version in the consumer repo and may fix any breaking changes.
4. Consume the new prop for the new needs.

This process is very long and risky. If finally the consumer finds the new prop/implementation does not fit the needs at step 4, the consumer needs to start over the whole process again. Besides, sometimes the step 3 is very hard, with version upgrade, it will introduce unwanted changes on unwanted components, sometimes it is hard to fix.

What I propose: create component with renderers (HoC, hooks, decorators, etc.) So the complicated components are integrations of renders and small components - with a few lines of codes. If the complicated components cannot fit the needs, the consumers can directly copy paste those few lines of codes to start customization. With a better situation, if there are many copy-paste on one very common scenario, fabric-react can normalize that requirement as one-stop component.

I've been thinking of a couple of ideas and have prototyped one already.
One option is to limit "how" to component creation (bind time) so that it doesn't mix in with render functions, but I worry that's a bit too limiting.
Another option is to separate out the "how" from the "what" props with a new slots prop (setting aside the use of inline objects):

First of all, your options are solving another problem different than my one described above. But, if I am making judgements on the options to solve my above problem, I could like to choose the how to render way. Because that gives more rooms for customization.

Besides, only providing how to render is not enough, the complicated component needs to be flat (with few lines of code) - it is unacceptable to copy-paste hundreds lines of code.

[lijunle]

BTW, @dzearing mentioned there is a createComponent utility to help on extend components. Is there any more doc on it? That utility may help on solving my problem.

[JasonGore]

Our slots work is intended to fix exactly the problem you just mentioned: each component is composed of simple, atomic functional regions which are called slots. Any user of said component can completely replace or supplement any or all slots with the own render functions and components.

The goal is to make customization easy and remove the need for bulky components that have to solve every scenario. We are already working on a new version of Button that has a simple Button core with variants along the lines of what you describe that add Split and Menu functionality as separate variants using slots.

The API changes in this PR directly affect createComponent. It hasn't been completely documented yet since we are finalizing the API for Fabric 7. It is similar to styled utility in concept and includes Slots & Tokens support.

Slot props can also be specified at binding time, the idea being that what you wrote above could be a new component variant also written like (tweaked to show a hypothetical where a Button that didn't have an Icon could have an IconButton variant):

// Let's create a component variant based on a Fabric component, or even another variant.
// extendComponent should allow users to easily create variants with just a few lines of code, like this:
const IconButton = extendComponent(Button, 
  content: {
    render: (props, DefaultComponent): JSX => {
      return (
        <>
          <Icon iconName={props.iconName} />
          <DefaultComponent {...props} />);
        </>
      );
    }
 }
};

function App() {
  const [inputValue, setInputValue] = React.useState('');
  return (
    <form>
      <input value={inputValue} onChange={setInputValue} />
      <IconButton content={{ iconName: 'Submit' }} />  // type safety will be the trick here
    <form>
  );
}

This all being said, I thought you were highlighting concerns with the slots API. These slot props don't have to be hidden inside of bulky components and their render functions, they can also be used to create component variants as shown above. If I'm still not addressing your concerns, maybe a call or meeting would be the next best step. 😄

[lijunle]

It is great to see that, my concerns have already been considered! 👍

However, I am not sure how slots are solving the customization problem. I read the slots implementation in this pull request, but not really understand it.

Besides, during looking on that PR, I have one question about this code:

const Slots = getSlots<typeof props, IButtonSlots>(props, {
  root: _deriveRootType(props),
  stack: Stack,
  icon: Icon,
  content: Text,
  menu: ContextualMenu,
  menuIcon: Icon
});
return (
  <Slots.root {...} />
);

Is the Slots.root memorized from the getSlots call? If no, it gets to the same problem as in #8834.

[JasonGore]

Regarding the customization problem, I gave an example of how an IconButton variant could be made using a simple Button core. Other options include replacing the content entirely, not just supplementing it. What customization options do you see missing? Can you modify that example to show?

Slots.root is not actually a component and does not go through React.createElement. There is a @jsx withSlots helper that resolves Slots.root handlers. The handlers render the actual content, which by default is the content in the object passed to getSlots and is memoized internally. As you said, without the withSlots helper, it would rerender on every call. (And there is an error generated if slot handlers are used without withSlots, as well.)

[msft-github-bot]

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Official NPM Release