React: Fix performance issue with useTopLayer in causing re-renders of all consumers on page whenever a single one changes#3662
React: Fix performance issue with useTopLayer in causing re-renders of all consumers on page whenever a single one changes#3662rkoval wants to merge 1 commit intotailwindlabs:mainfrom
useTopLayer in causing re-renders of all consumers on page whenever a single one changes#3662Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
hey @RobinMalfait - i would love your thoughts here at your earliest convenience! this issue seems like this is related to this PR, so your guidance would be greatly appreciated |
|
Hey! Thanks for your work on this problem. I went with another solution implemented in #3722 I think the implementation where you can control the What I ended up doing instead, is make sure that the hook only re-renders the component if the result actually changes. This is using a This means that only 2 components at most should re-render:
All the other components were already in the |
Fixes #3630
First off, Headless UI is great!! I love that I have full control over the visual display of all components within. Thank you so much for your effort in creating and maintaining this!! 🙏
My understanding of the problem:
All of the current
useOutsideClickcomponents (i.e., Menu, Combobox, Dialog, Listbox, and Popover) rely on the underlyinguseIsTopLayerhook to know what position of the visual stack each component. As each component renders, they are pushed on top of thisuseIsTopLayerstack. This stack is then used to determine which component to dismiss when clicking off of the menu or pressing "Escape". This is explained better in this comment.However, the problem is that each component gets pushed into the stack even if the menu is not being displayed on screen; it is the component trigger that determines what gets added or removed from the stack because it applies to the top-level component. For example, you still must render a top-level
Popovercomponent to wrapPopoverButtonevenPopoverPanelwill not be visible untilPopoverButtonis interacted with. This becomes problematic becauseuseOutsideClickuses the same bucket key internally for all components that consume it. That means that every time a consuming component renders, it causes theuseIsTopLayerbucket to add/remove, which triggers re-renders from every component that usesuseOutsideClick. If you have a lot of these components on screen at once (e.g., think having a table with context menus for each row), this can slow down page performance significantly, especially for lower end devicesMy proposed solution:
For each of the consuming components, expose a
outsideClickScopeprop that will allow passing a scope down intouseIsTopLayerso that each one has its own bucket/stack. If you have menus that require knowledge of other menus to be in the same stack for closing (e.g., aListboxwithin aDialog), you must pass in the same scope key for both components so that they know how to interact.This solves our immediate problem, though it does not feel like the most elegant solution for the following reasons:
Dialog), so if you have many of those in the same scope, the same problem is still present.Dialoguse case, you must have some context or mechanism that knows when you need to pass in a specific scope so that the outside click logic knows the order by which to hide the components.All of this said, this solution will solve all practical cases outside of the nested use case, and it's only one I can think of without a broader refactoring of how
useOutsideClick/useIsTopLayerworks. Being that I am a first time contributor to this codebase, I know that I do not have the context necessary to make such a sweeping change efficiently and effectively 😅.If we want to hold on this PR for a broader/better solution to be implemented, I would love for it to at least be merged as undocumented so that immediate usage is unblocked, and we don't have to maintain our own fork.
I am open to thoughts for how best to proceed!