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.

Sign up for GitHub

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

Jump to bottom

Prevent unnecessary execution of the displayValue callback in the ComboboxInput component #3048

Merged
merged 3 commits into from Mar 21, 2024
Merged

Prevent unnecessary execution of the displayValue callback in the ComboboxInput component #3048

merged 3 commits into from Mar 21, 2024

Conversation

RobinMalfait
Copy link
Collaborator

This PR fixes an issue where the displayValue on the ComboboxInput component is executed on every render. This PR fixes that by only executing the callback if either the value or the displayValue itself changes.

This is especially useful if you have a displayValue that is a function that does some heavy computation or that contains undeterministic code. E.g.: Math.random(), Date.now(), …

Fixes: #3044

@RobinMalfait
memoize the currentDisplayValue
ff18a52 
This used to be re-executed every single render. This should typically
not be an issue, but if you use non-deterministic code (E.g.:
`Math.random`, `Date.now`, …) then it could result in incorrect values.

Using `useMemo` allows us to only re-run it if the `data.value` or thte
`displayValue` actually changes.
@RobinMalfait
add test to verify currentDisplayValue is stable
30dcb86
@vercel Vercel
Copy link

vercel bot commented Mar 20, 2024 •
edited

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 Mar 20, 2024 11:34pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 20, 2024 11:34pm

@RobinMalfait RobinMalfait changed the title Prevent unnecessary execution of displayValue callback Prevent unnecessary execution of the displayValue callback Mar 20, 2024
@RobinMalfait RobinMalfait changed the title Prevent unnecessary execution of the displayValue callback Prevent unnecessary execution of the displayValue callback in the ComboboxInput component Mar 20, 2024
@thecrypticace
Copy link
Contributor

thecrypticace commented Mar 21, 2024 •
edited

We used to useMemo for displayValue but changed it. The big difference here is the dependency array though. We'll want to make sure this doesn't break: #2090 (this is the PR where we changed it to the current impl)

@RobinMalfait
Copy link
Collaborator Author

We used to useMemo for displayValue but changed it. The big difference here is the dependency array though. We'll want to make sure this doesn't break: #2090 (this is the PR where we changed it to the current impl)

Aha good catch! But yep just updated the useMemo dependencies array where I removed the displayValue and that does indeed break the previous issue. Adding it though, makes the test pass.

When running: clear; git diff; npm run test combobox; git reset --hard; npm run test combobox you can see that the displayValue as a dependency is important.

image

@RobinMalfait RobinMalfait merged commit 000e0c0 into main Mar 21, 2024
8 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-3044 branch March 21, 2024 12:46
@RobinMalfait RobinMalfait mentioned this pull request Mar 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thecrypticace thecrypticace thecrypticace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Combobox: input defaultValue change when displayValue change
2 participants
@RobinMalfait @thecrypticace