Improve the `slots` pattern and implementation

[iansan5653]

Summary

This PR contains a number of changes and improvements to the 'slots' pattern (createSlots) to fix performance issues, reduce complexity, and improve the API:

Closes https://github.com/github/primer/issues/1224

Remove context prop from Slots

The ability to pass custom non-slots-related contexts through the slots API is problematic:

• It is not typesafe (types are any going in and coming out)
• It breaks memoization - the slots context and components cannot be memoized if a custom context object that changes on every render is passed
• It is a violation of single-responsibility principle - slots should manage slots and nothing else
• It increases complexity in the already-complex createSlots function

Because of these issues, particularly the memoization problem, I removed the custom context capabilities altogether. The missing functionality is easily solved by creating new contexts using React.createContext, which follows the single-responsibility principle and is typesafe.

Use normal React state to manage slots

Currently we are storing the slots object in a ref and forcing updates every time it changes. The idea behind this is that we can avoid extra renders on mount, when a number of slots are registering all at once. In reality this actually drastically hurt performance because React cannot batch these updates. By moving the store into normal state, we can simply update state and let React decide when to render.

This looks slightly different from normal useState because I'm using the slightly more low-level useReducer to clean up the code. useReducer also helps us avoid passing an object (which would have to be memoized) to SlotsContext.

Now that there is only one function being passed to SlotsContext (and that function is guaranteed to never change because it's a reducer), SlotsContext will never change - this means fewer extra renders!

Memoize the Slot component

Even with the context value never changing, we still see extra renders in the Slots components themselves. This is because React re-renders the entire subtree when state changes, and Slots change state when they render - causing them to render again. We can easily fix this by memoizing the Slot component with memo. Now it will not re-render unless its children change - that's only once per 'real' render.

Split apart the register & unregister effects

We are currently unnecessarily unregistering each slot before we register it. This is not needed because slots just overwrite the store entry. We do still need to unregister on dismount, but that can have a smaller dependency list - thus fewer state updates!

This actually has no practical effect now that we are using state to store the slots; previously it had a major impact because we were forcing a render on unregister and on register. But now React would just batch the state updates anyway. However, I don't think we should rely on React batching things under the hood - we should still avoid unnecessary updates.

Change to a hook-based API

Currently we generate a Slots provider component with createSlots, which provides the slots through a render function. This render function is hard to read, and more importantly it doesn't provide access to the slots record at the top-level. We often want to access this object to make decisions based on whether a slot exists or not, so this is a very cumbersome API:

const {Slots, Slot} = createSlots<"One" | "Two">();
export const ItemSlot = Slot;

export const Item = ({children}) => {
  return (
    <Slots>
      {(slots) => {
        const oneExists = slots.One !== undefined;

        return (
          <>
            {oneExists ? "One Exists" : null}
            {slots.One}

            {children}
            {slots.Two}
          </>
        );
      }}
    </Slots>
  );
};

To improve this, I've changed the API to generate a useSlots hook instead. When called inside a component, that hook returns the slots record and a SlotsProvider component, which acts like a normal context provider (but with no props). In practice, this looks like this:

const {useSlots, Slot} = createSlots<"One" | "Two">();
export const ItemSlot = Slot;

export const Item = ({children}) => {
  const {SlotsProvider, slots} = useSlots();

  const oneExists = slots.One !== undefined;

  return (
    <SlotsProvider>
      {oneExists ? "One Exists" : null}
      {slots.One}

      {children}
      {slots.Two}
    </SlotsProvider>
  );
};

This should have no performance impact, but I think it's much more readable and ergonomic.

Screenshots

Here's a quick demo showing how many fewer renders we are seeing now vs before:

Before: Screen recording of a console logging every Slot render and every top-level Slots provider render in the MarkdownEditor component. Everything renders three times on initial mount, then each time the value of the input changes, everything renders twice.

https://user-images.githubusercontent.com/2294248/185676730-6876e905-6bb9-4b1f-bc16-e401e3b2ddd9.mov

After: A similar recording, after the changes have been made. Each Slot renders once on initial mount and once per change. The top-level provider renders twice on mount and twice per change.

https://user-images.githubusercontent.com/2294248/185678092-05efbc68-8078-44ab-8f79-aa4016316b15.mov

Of course, number of renders alone is not a good indicator of performance, but it does show that we've been able to simplify the event cycle here which should help resolve the edge cases we've seen. And at the same time, we've been able to improve the API, so it's a win-win.

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• [ ] Tested in Chrome
• [ ] Tested in Firefox
• [ ] Tested in Safari
• [ ] Tested in Edge

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 1636e08b41c2453bc715b56575645d9904bbb11f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions[bot]]

size-limit report 📦

Path	Size
dist/browser.esm.js	101.52 KB (+33.14% 🔺)
dist/browser.umd.js	101.86 KB (+32.53% 🔺)

[iansan5653]

Looks like some components (but not all 🤔) using slots are failing to render due to infinite update depth. Not sure what's going on there.

[siddharthkp]

Hi @iansan5653!

We discussed this in the engineering sync and heads up, it might be a while! We have decided to slow down and do some explorations before committing to a solution

[siddharthkp]

Converting this into a draft, purely for logistics 😅

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

[iansan5653]

@siddharthkp should we just let this close? I still think this change would solve a lot of headaches and fix performance issues. Is it worth the effort to review and try it out?

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.