Ensure by comparison is used in multiple mode (#1717)

* use the `compare` function in multiple mode * add tests to verify fix of incorrect `by` behaviour * improve TypeScript types for the `by` prop * update changelog
tailwindlabs · Jul 26, 2022 · 6598db5 · 6598db5
1 parent f2c2d3c
commit 6598db5
Show file tree

Hide file tree

Showing 9 changed files with 330 additions and 15 deletions.
diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482))
+- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482), [#1717](https://github.com/tailwindlabs/headlessui/pull/1717))
 - Add `@headlessui/tailwindcss` plugin ([#1487](https://github.com/tailwindlabs/headlessui/pull/1487))
 
 ### Fixed

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
@@ -272,6 +272,84 @@ describe('Rendering', () => {
           )
         })
       )
+
+      it(
+        'should be possible to use completely new objects while rendering (single mode)',
+        suppressConsoleLogs(async () => {
+          function Example() {
+            let [value, setValue] = useState({ id: 2, name: 'Bob' })
+
+            return (
+              <Combobox value={value} onChange={setValue} by="id">
+                <Combobox.Button>Trigger</Combobox.Button>
+                <Combobox.Options>
+                  <Combobox.Option value={{ id: 1, name: 'alice' }}>alice</Combobox.Option>
+                  <Combobox.Option value={{ id: 2, name: 'bob' }}>bob</Combobox.Option>
+                  <Combobox.Option value={{ id: 3, name: 'charlie' }}>charlie</Combobox.Option>
+                </Combobox.Options>
+              </Combobox>
+            )
+          }
+
+          render(<Example />)
+
+          await click(getComboboxButton())
+          let [alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+
+          await click(getComboboxOptions()[2])
+          await click(getComboboxButton())
+          ;[alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).not.toHaveAttribute('aria-selected')
+          expect(charlie).toHaveAttribute('aria-selected', 'true')
+
+          await click(getComboboxOptions()[1])
+          await click(getComboboxButton())
+          ;[alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+        })
+      )
+
+      it(
+        'should be possible to use completely new objects while rendering (multiple mode)',
+        suppressConsoleLogs(async () => {
+          function Example() {
+            let [value, setValue] = useState([{ id: 2, name: 'Bob' }])
+
+            return (
+              <Combobox value={value} onChange={setValue} by="id" multiple>
+                <Combobox.Button>Trigger</Combobox.Button>
+                <Combobox.Options>
+                  <Combobox.Option value={{ id: 1, name: 'alice' }}>alice</Combobox.Option>
+                  <Combobox.Option value={{ id: 2, name: 'bob' }}>bob</Combobox.Option>
+                  <Combobox.Option value={{ id: 3, name: 'charlie' }}>charlie</Combobox.Option>
+                </Combobox.Options>
+              </Combobox>
+            )
+          }
+
+          render(<Example />)
+
+          await click(getComboboxButton())
+
+          await click(getComboboxOptions()[2])
+          let [alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).toHaveAttribute('aria-selected', 'true')
+
+          await click(getComboboxOptions()[2])
+          ;[alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+        })
+      )
     })
   })
 

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -315,7 +315,7 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
   > & {
     value: TType
     onChange(value: TType): void
-    by?: (keyof TType & string) | ((a: TType, z: TType) => boolean)
+    by?: (keyof TActualType & string) | ((a: TActualType, z: TActualType) => boolean)
     disabled?: boolean
     __demoMode?: boolean
     name?: string
@@ -356,19 +356,19 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
 
   let compare = useEvent(
     typeof by === 'string'
-      ? (a: TType, z: TType) => {
-          let property = by as unknown as keyof TType
+      ? (a: TActualType, z: TActualType) => {
+          let property = by as unknown as keyof TActualType
           return a[property] === z[property]
         }
       : by
   )
 
-  let isSelected: (value: TType) => boolean = useCallback(
+  let isSelected: (value: TActualType) => boolean = useCallback(
     (compareValue) =>
       match(data.mode, {
         [ValueMode.Multi]: () =>
-          (value as unknown as TType[]).some((option) => compare(option, compareValue)),
-        [ValueMode.Single]: () => compare(value, compareValue),
+          (value as unknown as TActualType[]).some((option) => compare(option, compareValue)),
+        [ValueMode.Single]: () => compare(value as unknown as TActualType, compareValue),
       }),
     [value]
   )
@@ -500,7 +500,7 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
       [ValueMode.Multi]() {
         let copy = (data.value as TActualType[]).slice()
 
-        let idx = copy.indexOf(value as TActualType)
+        let idx = copy.findIndex((item) => compare(item, value as TActualType))
         if (idx === -1) {
           copy.push(value as TActualType)
         } else {

diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx
@@ -263,6 +263,84 @@ describe('Rendering', () => {
           )
         })
       )
+
+      it(
+        'should be possible to use completely new objects while rendering (single mode)',
+        suppressConsoleLogs(async () => {
+          function Example() {
+            let [value, setValue] = useState({ id: 2, name: 'Bob' })
+
+            return (
+              <Listbox value={value} onChange={setValue} by="id">
+                <Listbox.Button>Trigger</Listbox.Button>
+                <Listbox.Options>
+                  <Listbox.Option value={{ id: 1, name: 'alice' }}>alice</Listbox.Option>
+                  <Listbox.Option value={{ id: 2, name: 'bob' }}>bob</Listbox.Option>
+                  <Listbox.Option value={{ id: 3, name: 'charlie' }}>charlie</Listbox.Option>
+                </Listbox.Options>
+              </Listbox>
+            )
+          }
+
+          render(<Example />)
+
+          await click(getListboxButton())
+          let [alice, bob, charlie] = getListboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+
+          await click(getListboxOptions()[2])
+          await click(getListboxButton())
+          ;[alice, bob, charlie] = getListboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).not.toHaveAttribute('aria-selected')
+          expect(charlie).toHaveAttribute('aria-selected', 'true')
+
+          await click(getListboxOptions()[1])
+          await click(getListboxButton())
+          ;[alice, bob, charlie] = getListboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+        })
+      )
+
+      it(
+        'should be possible to use completely new objects while rendering (multiple mode)',
+        suppressConsoleLogs(async () => {
+          function Example() {
+            let [value, setValue] = useState([{ id: 2, name: 'Bob' }])
+
+            return (
+              <Listbox value={value} onChange={setValue} by="id" multiple>
+                <Listbox.Button>Trigger</Listbox.Button>
+                <Listbox.Options>
+                  <Listbox.Option value={{ id: 1, name: 'alice' }}>alice</Listbox.Option>
+                  <Listbox.Option value={{ id: 2, name: 'bob' }}>bob</Listbox.Option>
+                  <Listbox.Option value={{ id: 3, name: 'charlie' }}>charlie</Listbox.Option>
+                </Listbox.Options>
+              </Listbox>
+            )
+          }
+
+          render(<Example />)
+
+          await click(getListboxButton())
+
+          await click(getListboxOptions()[2])
+          let [alice, bob, charlie] = getListboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).toHaveAttribute('aria-selected', 'true')
+
+          await click(getListboxOptions()[2])
+          ;[alice, bob, charlie] = getListboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+        })
+      )
     })
   })
 

diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx
@@ -315,7 +315,7 @@ let ListboxRoot = forwardRefWithAs(function Listbox<
   > & {
     value: TType
     onChange(value: TType): void
-    by?: (keyof TType & string) | ((a: TType, z: TType) => boolean)
+    by?: (keyof TActualType & string) | ((a: TActualType, z: TActualType) => boolean)
     disabled?: boolean
     horizontal?: boolean
     name?: string
@@ -345,8 +345,8 @@ let ListboxRoot = forwardRefWithAs(function Listbox<
         mode: multiple ? ValueMode.Multi : ValueMode.Single,
         compare: useEvent(
           typeof by === 'string'
-            ? (a: TType, z: TType) => {
-                let property = by as unknown as keyof TType
+            ? (a: TActualType, z: TActualType) => {
+                let property = by as unknown as keyof TActualType
                 return a[property] === z[property]
               }
             : by
@@ -377,7 +377,10 @@ let ListboxRoot = forwardRefWithAs(function Listbox<
         [ValueMode.Multi]() {
           let copy = (propsRef.current.value as TActualType[]).slice()
 
-          let idx = copy.indexOf(value as TActualType)
+          let { compare } = propsRef.current
+          let idx = copy.findIndex((item) =>
+            compare(item as unknown as TActualType, value as TActualType)
+          )
           if (idx === -1) {
             copy.push(value as TActualType)
           } else {

diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482))
+- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482), [#1717](https://github.com/tailwindlabs/headlessui/pull/1717))
 - Add `@headlessui/tailwindcss` plugin ([#1487](https://github.com/tailwindlabs/headlessui/pull/1487))
 
 ### Fixed

diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts
@@ -308,6 +308,84 @@ describe('Rendering', () => {
           )
         })
       )
+
+      it(
+        'should be possible to use completely new objects while rendering (single mode)',
+        suppressConsoleLogs(async () => {
+          renderTemplate({
+            template: html`
+              <Combobox v-model="value" by="id">
+                <ComboboxButton>Trigger</ComboboxButton>
+                <ComboboxOptions>
+                  <ComboboxOption :value="{ id: 1, name: 'alice' }">alice</ComboboxOption>
+                  <ComboboxOption :value="{ id: 2, name: 'bob' }">bob</ComboboxOption>
+                  <ComboboxOption :value="{ id: 3, name: 'charlie' }">charlie</ComboboxOption>
+                </ComboboxOptions>
+              </Combobox>
+            `,
+            setup: () => {
+              let value = ref({ id: 2, name: 'Bob' })
+              return { options, value }
+            },
+          })
+
+          await click(getComboboxButton())
+          let [alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+
+          await click(getComboboxOptions()[2])
+          await click(getComboboxButton())
+          ;[alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).not.toHaveAttribute('aria-selected')
+          expect(charlie).toHaveAttribute('aria-selected', 'true')
+
+          await click(getComboboxOptions()[1])
+          await click(getComboboxButton())
+          ;[alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+        })
+      )
+
+      it(
+        'should be possible to use completely new objects while rendering (multiple mode)',
+        suppressConsoleLogs(async () => {
+          renderTemplate({
+            template: html`
+              <Combobox v-model="value" by="id" multiple>
+                <ComboboxButton>Trigger</ComboboxButton>
+                <ComboboxOptions>
+                  <ComboboxOption :value="{ id: 1, name: 'alice' }">alice</ComboboxOption>
+                  <ComboboxOption :value="{ id: 2, name: 'bob' }">bob</ComboboxOption>
+                  <ComboboxOption :value="{ id: 3, name: 'charlie' }">charlie</ComboboxOption>
+                </ComboboxOptions>
+              </Combobox>
+            `,
+            setup: () => {
+              let value = ref([{ id: 2, name: 'Bob' }])
+              return { options, value }
+            },
+          })
+
+          await click(getComboboxButton())
+
+          await click(getComboboxOptions()[2])
+          let [alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).toHaveAttribute('aria-selected', 'true')
+
+          await click(getComboboxOptions()[2])
+          ;[alice, bob, charlie] = getComboboxOptions()
+          expect(alice).not.toHaveAttribute('aria-selected')
+          expect(bob).toHaveAttribute('aria-selected', 'true')
+          expect(charlie).not.toHaveAttribute('aria-selected')
+        })
+      )
     })
   })
 

diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts
@@ -316,7 +316,7 @@ export let Combobox = defineComponent({
               let copy = toRaw(api.value.value as unknown[]).slice()
               let raw = toRaw(dataRef.value)
 
-              let idx = copy.indexOf(raw)
+              let idx = copy.findIndex((value) => api.compare(raw, toRaw(value)))
               if (idx === -1) {
                 copy.push(raw)
               } else {
@@ -341,7 +341,7 @@ export let Combobox = defineComponent({
               let copy = toRaw(api.value.value as unknown[]).slice()
               let raw = toRaw(dataRef.value)
 
-              let idx = copy.indexOf(raw)
+              let idx = copy.findIndex((value) => api.compare(raw, toRaw(value)))
               if (idx === -1) {
                 copy.push(raw)
               } else {