Improve or replace `Slots` pattern/implementation

[iansan5653]

Context

ADR 004 describes the preferred children-based API approach for React components, where most rendered content is passed to a component through the children prop.

When components want to define specific 'chunks' of children, they use our homebrewed slots internal API to define wrapper components that consumers can use like props. An example from the ADR uses the ActionList.LeadingVisual slot to define a piece of the children that becomes the leading visual for the item:

<ActionList.Item>
  <ActionList.LeadingVisual>
    <Avatar src="https://github.com/mona.png" />
  <ActionList.LeadingVisual>
  mona
</ActionList.Item>

This strategy makes for a flexible and readable component API. It's very clear here what will end up as the leading visual, and it's plain old JSX so any JSX content can be used. From the point of view of the API consumer, this looks great.

Issues

Not idiomatic

The first issue (and really the source of other issues) with the slots pattern is simply that it isn't idiomatic React. The above example has exactly the same effect as defining a leadingVisual prop with a ReactNode type, but instead we are abusing the children prop to provide that data. This aligns more closely with traditional HTML (ie select & option, or fieldset & label), but it's not a pattern that's natively supported by React, and that makes it challenging to implement effectively.

Implementation

The implementation of slots under the hood is problematic, and there's no obvious way to make it better - this is a natural effect of trying to do something React wasn't designed to do. Behind the scenes, a Slot will cause the parent Slots component to render at least three times every time it's own children changes:

react/src/utils/create-slots.tsx

Lines 93 to 96 in 4badb57

	useLayoutEffect(() => {
	registerSlot(name, typeof children === 'function' ? children(context) : children)
	return () => unregisterSlot(name)
	}, [name, children, registerSlot, unregisterSlot, context])

1. children changes, causing a render
2. layout effect cleanup runs, calling unregisterSlot and eventually forces an update, causing another render
3. layout effect body runs, calling registerSlot and forcing another update

Because this happens in a layoutEffect, there should only be one paint per render, but this still has performance implications and can result in really strange issues that can be very hard to reason with.

In addition, the createSlots code itself is just really hard to understand and maintain. It works very differently from how React traditionally functions, sending data up the component tree instead of down.

Type safety

Finally, we can't enforce any sort of type safety on these components. React children aren't very typesafe in the first place, but slots make it worse. As an example, look at FormControl. If no FormControl.Label child is found, we log an error at runtime. The implementation of this is complex and error prone - ie, the following will error because no FormControl.Label direct child is found, even though it results in valid HTML:

const MyLabel = () => <FormControl.Label>My Label</FormControl.Label>

const Errors = () => (
  <FormControl>
    <MyLabel />
  </FormControl>
)

If we could enforce the requirement through the type system, there would be no need to have this complex runtime logic.

Potential solutions

Extract children before rendering

I did try to rewrite the slots implementation in such a way that it doesn't require any additional renders, by extracting the slots from the children using a hook. In practice, this would look like this:

const Component = ({children}) => {
  const {slots, remainingChildren} = useSlots(children)
  
  return <>
    {slots.label}
    {remainingChildren}
  </>
}

However, this is (as far as I can tell) impossible to implement. You can recursively walk the children tree to obtain all child nodes and all children props of child nodes, but you can't obtain all nodes. For example, the following will only be visible in a children tree as ComponentX, because even though ComponentY will get rendered, it's not in ComponentX's children:

const ComponentX = () => <ComponentY />

Unfortunately there's no way around this - we'd want to extract the nodes before rendering, but we don't know what the return value of a function (component) is until we call (render) it.

Use Portals

This is just an idea at this point, but the idiomatic React way to render something into a different location in the tree is to use portals. Maybe we can take advantage of this functionality to avoid extra renders?

Update: I've tried this and I couldn't get it to work. Open to ideas here though.

Improve the current implementation

It's possible we could improve the current implementation enough to work. Can we reduce the number of renders? Can we minimize the number of times a slot is registered or unregistered? This might just require some experimentation.

Just use props

Most use-cases of slots could be covered by simply using traditional React props. For example, the ActionList example from above would work the same way but be more performant and straightforward if it was defined in this manner:

<ActionList.Item leadingVisual={<Avatar src="https://github.com/mona.png" />}>
  mona
</ActionList.Item>

This has many advantages:

• It's idiomatic React code (recommended in the official docs)
• It doesn't require any extra renders
• It doesn't require any complex logic
• The content is accessible within the component body and can be handled just like regular children
• The prop can be marked as required, forcing the content to be defined
• It's slightly shorter 🤷‍♂️

This is not to say we should never have subcomponents like ActionList.Item - that's not a slot and it's still a very useful pattern. But in cases where we do use slots today, props would be a better alternative.

Tradeoffs

The primary tradeoff here (besides the arguably less readable syntax) is that there's no way to define the JSX and also define props for that slot. For example, FormControl.Label can take a visuallyHidden prop and it would be cumbersome to have label and labelVisuallyHidden as top-level props. These cases are actually pretty rare, however, and I think there are typically creative solutions (such as having the user wrap the label in VisuallyHidden instead):

<FormControl label={<VisuallyHidden>Label</VisuallyHidden>} >
  ...
</FormControl>

Related links

• 🔍 Search for all current uses of createSlots
• 📝 ADR 004: Children as API
• 🧵 Slack thread about a tricky bug involving slots
• https://github.com/github/primer/issues/1224
• Recent PRs fixing tricky bugs inside slots (highlighting the unnecessary complexity with this pattern)
  • Fix slots infinite rendering when no context prop is provided #2219
  • Change createSlots to use layout effects when registering slots #2216

[iansan5653]

Updated to be a bit less prescriptive as I think there may be more than one good solution here

[iansan5653]

After a lot of thought and playing with the code, I'm going to close this issue in favor of discussing some ideas in the other issue (https://github.com/github/primer/issues/1224) and potentially just open a PR to fix it.