Skip to content

Commit

Permalink
Prevent unnecessary execution of the displayValue callback in the `…
Browse files Browse the repository at this point in the history
…ComboboxInput` component (#3048)

* memoize the `currentDisplayValue`

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.

* add test to verify `currentDisplayValue` is stable

* update changelog
  • Loading branch information
RobinMalfait committed Mar 21, 2024
1 parent 834dbf4 commit 000e0c0
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 2 deletions.
1 change: 1 addition & 0 deletions packages/@headlessui-react/CHANGELOG.md
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Forward `disabled` state to hidden inputs in form-like components ([#3004](https://github.com/tailwindlabs/headlessui/pull/3004))
- Prefer incoming `data-*` attributes, over the ones set by Headless UI ([#3035](https://github.com/tailwindlabs/headlessui/pull/3035))
- Respect `selectedIndex` for controlled `<Tab/>` components ([#3037](https://github.com/tailwindlabs/headlessui/pull/3037))
- Prevent unnecessary execution of the `displayValue` callback in the `ComboboxInput` component ([#3048](https://github.com/tailwindlabs/headlessui/pull/3048))

### Changed

Expand Down
Expand Up @@ -489,6 +489,53 @@ describe('Rendering', () => {
})
)

it(
'should keep the defaultValue when the Combobox state changes',
suppressConsoleLogs(async () => {
let data = [
{ id: 1, name: 'alice', label: 'Alice' },
{ id: 2, name: 'bob', label: 'Bob' },
{ id: 3, name: 'charlie', label: 'Charlie' },
]

function Example() {
let [person, setPerson] = useState(data[1])

return (
<Combobox value={person} onChange={setPerson} name="assignee" by="id">
<Combobox.Input displayValue={() => String(Math.random())} />
<Combobox.Button />
<Combobox.Options>
{data.map((person) => (
<Combobox.Option key={person.id} value={person}>
{person.label}
</Combobox.Option>
))}
</Combobox.Options>
</Combobox>
)
}

render(<Example />)

let value = getComboboxInput()?.value

// Toggle the state a few times combobox
await click(getComboboxButton())
await click(getComboboxButton())
await click(getComboboxButton())

// Verify the value is still the same
expect(getComboboxInput()?.value).toBe(value)

// Choose an option, which should update the value
await click(getComboboxOptions()[2])

// Verify the value changed
expect(getComboboxInput()?.value).not.toBe(value)
})
)

it(
'should close the Combobox when the input is blurred',
suppressConsoleLogs(async () => {
Expand Down
Expand Up @@ -1005,15 +1005,15 @@ function InputFn<
// which should always result in a string (since we are filling in the value of the text input),
// you don't have to use this at all, a more common UI is a "tag" based UI, which you can render
// yourself using the selected option(s).
let currentDisplayValue = (function () {
let currentDisplayValue = useMemo(() => {
if (typeof displayValue === 'function' && data.value !== undefined) {
return displayValue(data.value as unknown as TType) ?? ''
} else if (typeof data.value === 'string') {
return data.value
} else {
return ''
}
})()
}, [data.value, displayValue])

// Syncing the input value has some rules attached to it to guarantee a smooth and expected user
// experience:
Expand Down
42 changes: 42 additions & 0 deletions packages/@headlessui-vue/src/components/combobox/combobox.test.ts
Expand Up @@ -483,6 +483,48 @@ describe('Rendering', () => {
)
})

it(
'should not crash when a defaultValue is not given',
suppressConsoleLogs(async () => {
let data = [
{ id: 1, name: 'alice', label: 'Alice' },
{ id: 2, name: 'bob', label: 'Bob' },
{ id: 3, name: 'charlie', label: 'Charlie' },
]

renderTemplate({
template: html`
<Combobox v-model="person" name="assignee" by="id">
<ComboboxInput :displayValue="displayValue" />
<ComboboxButton />
<ComboboxOptions>
<ComboboxOption v-for="person in data" :key="person.id" :value="person">
{{ person.label }}
</ComboboxOption>
<ComboboxOptions>
</Combobox>
`,
setup: () => ({ person: ref(data[0]), data, displayValue: () => String(Math.random()) }),
})

let value = getComboboxInput()?.value

// Toggle the state a few times combobox
await click(getComboboxButton())
await click(getComboboxButton())
await click(getComboboxButton())

// Verify the value is still the same
expect(getComboboxInput()?.value).toBe(value)

// Choose an option, which should update the value
await click(getComboboxOptions()[1])

// Verify the value changed
expect(getComboboxInput()?.value).not.toBe(value)
})
)

it(
'should not crash when a defaultValue is not given',
suppressConsoleLogs(async () => {
Expand Down

0 comments on commit 000e0c0

Please sign in to comment.