Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of Combobox component #2574

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Jul 4, 2023

This PR improves the performance of the Combobox component in Vue by reducing the amount of work that has to be done.

A few things that are optimized in this PR:

  1. Optimize registering options (A) — When registering an option, we have to make sure that all our options are sorted based on the DOM order. This is necessary if you use the arrow keys to go to the previous/next option, and if you just added an option between 2 other options. This is now optimized to only do the sorting once every option is registered instead of triggering a re-render for every single option you render.

  2. Optimize registering options (B) — When a new option was registered, we created a new array by spreading the old array in ([...options, newOption]), now we just push to the array.

  3. Optimize going to an option — When you hover from option A to option B, then 2 events happen:

    1. handleLeave(option A) which runs goToOption(Nothing)
    2. handleEnter(option B) which runs goToOption(option B)

    These are 2 actions that will re-render the Combobox component, but only the last one is interesting when moving between options. The handleLeave(option A) is still necessary if you move away from an option to outside of the Combobox itself, but not in this scenario.

To summarize:

  • The first 2 optimizations will influence the initial load and searching for options.
  • The last optimization will influence the mouse usage by making it less laggy (it can still be slow if you are rendering a lot of options, but then a windowing library should be used in addition).

Fixes: #2571
Fixes: #2527

No need to spread here because this will slow things down tremendously.
If you add 5 options, then this will happen:

```js
let options = []
options = [...options, 1]
options = [...options, 2]
options = [...options, 3]
options = [...options, 4]
options = [...options, 5]
```

It's making a lot of copies and has to spread everything it just copied
again. Instead, let's just push to the array:

```js
let options = []
options.push(1)
options.push(2)
options.push(3)
options.push(4)
options.push(5)
```
When registering 50 options, we will re-sort the options based on the DOM
node position for every option that gets registered. This is overkill
and unnecessary. The re-sorting is useful if an option is injected
between 2 options. When we sort the full list for _every_ `registerOption`
call then the result after the last `registerOption` will be the same if
we only sorted after the last `registerOption` call only.

This will ensure that we are skipping a ton of work and only do the work
once at the end where it matters.
@vercel
Copy link

vercel bot commented Jul 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2023 9:01am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2023 9:01am

This will allow us to perform the task of `goToOption` once instead of
`n` times if they happen really fast after eachother.

This can happen when you are leaving option B and going to option A.
Then the following steps happen:

- Leaving Option B `goToOption(Nothing)`
- Entering Option A `goToOption(A)`

Both will re-render everything because the internal active option index
would be changed. But only the last `goToOption(A)` is the interesting
version here.

We could also move the `goToOption(Nothing)` to the `ComboboxOptions`
(wrapper) itself instead of adding these listeners on each individual
`ComboboxOption`. However, if you add an additional visual piece of DOM
above or between the options, hovering that would not leave the
`ComboboxOptions` and therefore the last `ComboboxOption` would still be
active which we don't want.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant