[React-ARIA] - Maximum update depth exceeded while building popover component with render props

[bstro]

I'm stumped! Looking for a little help here, not sure if I'm doing something wrong, or if this is a bug.

I'm trying to build a flexible Popover component by using the example for useOverlayTrigger as a jumping-off point. I am trying to make this a flexible component for other developers on my team and I'm attempting to use the render-props pattern so that other developers don't have to worry about hooking up all the React-ARIA stuff in order to use the Popover. Here's a simplified reproduction in CodeSandbox.

Upon clicking the popover trigger, I see: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

One way to clear the error (for debugging purposes) is to not attach the popoverRef by commenting out line 60. The popover will then correctly render, though I'm assuming we also lose a lot of React-ARIA functionality so it isn't a real fix.

Software	Version(s)
react-aria	^3.10.0
Browser	Chrome 94.0.4606.71
Operating System	Big Sur

[snowystinger]

I think if you are seeing it fixed when you do not attach a popoverRef, then you are causing something where the actual node that popoverRef is attached to keeps changing. It's a bit hard to follow all of the render props around, but you can see that you're generating a new node constantly if you put this line of code into your PopoverWithTrigger just after the useOverlayTrigger

  let prevTrigger = useRef();
  if (prevTrigger.current && triggerRef.current) {
    console.log('trigger', triggerRef.current.isSameNode(prevTrigger.current))
  }
  useEffect(() => {
    if (triggerRef.current) {
      // @ts-ignore
      prevTrigger.current = triggerRef.current;
    }
  }, [triggerRef.current]);

I did chase it down to this effect https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/overlays/src/useOverlayPosition.ts#L126, which has a dep on the triggerRef.current

[snowystinger]

@bstro Just wanted to check in and see where you had landed on this topic. Would love to hear more about what you've tried as well.

[bstro]

Thanks for checking in, @snowystinger! Other tasks took priority since the last we've spoken on this, and I am just now hopping back on. I read your article on slots, and the approach makes sense to me. I've felt the pain-points of each approach that you've described in that article. Do you happen to have a simple example of how you implemented the slots architecture? Conceptually it makes sense, but I'd love to see a concrete implementation if you have something nearby/on-hand. Thank you!

[snowystinger]

For sure, we have it in React Spectrum :) so we've been developing with it for a while now.

Here's slots themselves, the context
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/utils/src/Slots.tsx

Here's a usage example setting it up
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/dialog/src/Dialog.tsx#L63
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/dialog/src/Dialog.tsx#L97

Here's a component that makes use of slots

react-spectrum/packages/@react-spectrum/view/src/Content.tsx

Line 21 in 1e2b7f2

props = useSlotProps(props, 'content');

and here's an example where we expect a lot of content provided by a user so we don't want our slots to continue to propagate

react-spectrum/packages/@react-spectrum/view/src/Content.tsx

Line 31 in 1e2b7f2

<ClearSlots>

[bstro]

I've been exploring a bunch of different ideas and have landed on an approach that I'm feeling good about so far. Its basically a combination of tailwindcss+react aria+your slot prop approach (albeit heavily pared-down at the moment).

Here's a draft gist, forking some code from react-spectrum's Slots code. https://gist.github.com/bstro/65e7f5b3fb7743aa1fafe0d84657fdba

Here's a heavily abridged version of a hook that uses the above slot props implementation: https://gist.github.com/bstro/a3c4f0771635625b24322cf1e635b8ec

Here's a ButtonGroup component where the slot context is being prepared (see example in the followup comment below this): https://gist.github.com/bstro/90e4283afb7a8d11f084980d905a0540

This approach leverages a library called tailwindcss-classnames which provides type safety while using tailwindcss classes via the popular classnames api. Also, currently using some methods from Spectrum's implementation.

One drawback of my current implementation is that the overrides cannot apply to one variation or another (for instance, what if I want a different style override for the primary Button but not the secondary Button?) If I noodle on this more, I'm sure I can smooth and improve that aspect of the API.

I'd love to hear your thoughts if/whenever you have a moment :)

[bstro]

The result, btw, of the above code examples, is that given the following button with rounded corners on all sides:

I am able to write the following JSX:

<ButtonGroup>
  <Button>Foo</Button>
  <Button>Foo</Button>
  <Button>Foo</Button>
</ButtonGroup>

which renders as this:

As you can see, the Buttons inside a ButtonGroup have the correct borderRadius applied without the caller having to specify anything at all, and no need for any kind of duplication like <ButtonGroup.Button> might entail.