Repository: tailwindlabs/headlessui
Pull request #3048 (2024-03-20 23:31:03)

Prevent unnecessary execution of the `displayValue` callback in the `ComboboxInput` component
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


vercel[bot]: [vc]: #YcKM0WEoKpRehkx7q64ZOmsZH/+A3bDXTZwVSB7NmBc=:eyJpc01vbm9yZXBvIjp0cnVlLCJ0eXBlIjoiZ2l0aHViIiwicHJvamVjdHMiOlt7Im5hbWUiOiJoZWFkbGVzc3VpLXJlYWN0Iiwicm9vdERpcmVjdG9yeSI6InBsYXlncm91bmRzL3JlYWN0IiwiaW5zcGVjdG9yVXJsIjoiaHR0cHM6Ly92ZXJjZWwuY29tL3RhaWx3aW5kbGFicy9oZWFkbGVzc3VpLXJlYWN0L0NqYk5pOUQ1Nnh6eEVZcGVVcVljZ3NiWFB4ZHAiLCJwcmV2aWV3VXJsIjoiaGVhZGxlc3N1aS1yZWFjdC1naXQtZml4LWlzc3VlLTMwNDQtdGFpbHdpbmRsYWJzLnZlcmNlbC5hcHAiLCJuZXh0Q29tbWl0U3RhdHVzIjoiUEVORElORyIsImxpdmVGZWVkYmFjayI6eyJyZXNvbHZlZCI6MCwidW5yZXNvbHZlZCI6MCwidG90YWwiOjAsImxpbmsiOiJoZWFkbGVzc3VpLXJlYWN0LWdpdC1maXgtaXNzdWUtMzA0NC10YWlsd2luZGxhYnMudmVyY2VsLmFwcCJ9fSx7Im5hbWUiOiJoZWFkbGVzc3VpLXZ1ZSIsInJvb3REaXJlY3RvcnkiOiJwbGF5Z3JvdW5kcy92dWUiLCJpbnNwZWN0b3JVcmwiOiJodHRwczovL3ZlcmNlbC5jb20vdGFpbHdpbmRsYWJzL2hlYWRsZXNzdWktdnVlLzVjb29pdnNIUzZvZnV2NFl5ZVJySG92dG5aUWIiLCJwcmV2aWV3VXJsIjoiaGVhZGxlc3N1aS12dWUtZ2l0LWZpeC1pc3N1ZS0zMDQ0LXRhaWx3aW5kbGFicy52ZXJjZWwuYXBwIiwibmV4dENvbW1pdFN0YXR1cyI6IlBFTkRJTkciLCJsaXZlRmVlZGJhY2siOnsicmVzb2x2ZWQiOjAsInVucmVzb2x2ZWQiOjAsInRvdGFsIjowLCJsaW5rIjoiaGVhZGxlc3N1aS12dWUtZ2l0LWZpeC1pc3N1ZS0zMDQ0LXRhaWx3aW5kbGFicy52ZXJjZWwuYXBwIn19XX0=
**The latest updates on your projects**. Learn more about [Vercel for Git ↗︎](https://vercel.link/github-learn-more)

| Name | Status | Preview | Comments | Updated (UTC) |
| :--- | :----- | :------ | :------- | :------ |
| **headlessui-react** | 🔄 Building ([Inspect](https://vercel.com/tailwindlabs/headlessui-react/CjbNi9D56xzxEYpeUqYcgsbXPxdp)) | [Visit Preview](https://vercel.live/open-feedback/headlessui-react-git-fix-issue-3044-tailwindlabs.vercel.app?via=pr-comment-visit-preview-link&passThrough=1) | 💬 [**Add feedback**](https://vercel.live/open-feedback/headlessui-react-git-fix-issue-3044-tailwindlabs.vercel.app?via=pr-comment-feedback-link) | Mar 20, 2024 11:31pm |
| **headlessui-vue** | 🔄 Building ([Inspect](https://vercel.com/tailwindlabs/headlessui-vue/5cooivsHS6ofuv4YyeRrHovtnZQb)) | [Visit Preview](https://vercel.live/open-feedback/headlessui-vue-git-fix-issue-3044-tailwindlabs.vercel.app?via=pr-comment-visit-preview-link&passThrough=1) | 💬 [**Add feedback**](https://vercel.live/open-feedback/headlessui-vue-git-fix-issue-3044-tailwindlabs.vercel.app?via=pr-comment-feedback-link) | Mar 20, 2024 11:31pm |



thecrypticace: 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: https://github.com/tailwindlabs/headlessui/pull/2090
RobinMalfait: > 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.

<img width="1817" alt="image" src="https://github.com/tailwindlabs/headlessui/assets/1834413/c5b1b122-0d09-4df1-835d-a062c78a73cd">


Patch:
diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
index 5541baa..73b5e86 100644
--- a/packages/@headlessui-react/CHANGELOG.md
+++ b/packages/@headlessui-react/CHANGELOG.md
@@ -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
 
diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
index 7971322..1c4c88f 100644
--- a/packages/@headlessui-react/src/components/combobox/combobox.tsx
+++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -1005,7 +1005,7 @@ 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') {
@@ -1013,7 +1013,7 @@ function InputFn<
     } else {
       return ''
     }
-  })()
+  }, [data.value, displayValue])
 
   // Syncing the input value has some rules attached to it to guarantee a smooth and expected user
   // experience:


Test patch:
diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
index 562a33e..8e24d69 100644
--- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
+++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
@@ -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 () => {
diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts
index ee56f2d..d1e7772 100644
--- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts
+++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts
@@ -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 () => {