[Combobox] `onChange` type incorrectly determined

[JesusTheHun]

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.6.1

What browser are you using?

N/A

Reproduction URL

https://stackblitz.com/edit/react-ts-jldzdg?file=MyCombobox.tsx

Describe your issue

The Combobox component determined the type of the argument of the onChange function solely based on the type of the value property it receives. Unfortunately that is not accurate nor safe.

// 1. Nullable
<Combobox value={'hi'} onChange={(selection) => console.log("changed")} nullable={true} />

// Wrong type
type SelectionActual = string;
type SelectionExpected = string | null;

// 2. Multiple

/* 
Looking at the two cases below you cannot tell if there should be an error or not.
Maybe the option value is a `[string, string]`and maybe it's just a `string`.
Also, when `multiple & nullable = true` then `selection` cannot be null. So the type should reflect that.
*/

<Combobox value={['a', 'b']} onChange={(selection) => console.log("changed")} nullable={true} multiple={false} />
<Combobox value={['a', 'b']} onChange={(selection) => console.log("changed")} nullable={true} multiple={true} />

[RobinMalfait]

Hey! Thank you for your suggestion! Much appreciated! 🙏

We can definitely update some of the types to keep the nullable prop into account if multiple = false. However both cases here are valid, so there is nothing we can do here:

<Combobox value={['a', 'b']} onChange={(selection) => console.log("changed")} nullable={true} multiple={false} />
<Combobox value={['a', 'b']} onChange={(selection) => console.log("changed")} nullable={true} multiple={true} />

The only thing that is invalid would be this:

<Combobox value={'a'} onChange={(selection) => console.log("changed")} nullable={true} multiple={true} />

Because when multiple is set to true we definitely know that it should be an array.

The reason both those cases are valid is that it could be that you are storing a single value where the value is a tuple, which is why we make the multiple prop explicit instead of trying to infer the value:

<Combobox value={['a', 'b']} onChange={(selection) => console.log("changed")} nullable={true} multiple={false} />

I'll keep this open until we improved the types based on the nullabel and multiple prop.

[JesusTheHun]

Hi @RobinMalfait, you are right in the case of multiple, we cannot infer, that's why the Combobox component should accept a generic that defines the type of the option value.

<Combobox<string> /> // string
<Combobox<string> nullable /> // string | null
<Combobox<string> nullable multiple /> // string[]

Right now the Combobox component has some generics but their order do not make things easy to overwrite them, also I'm not sure the TActualType is actually used :

function Combobox<
  TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
  TType = string,
  TActualType = TType extends (infer U)[] ? U : TType
>();

[RobinMalfait]

This should be fixed by #1761, and will be available in the next release.

You can already try it using npm install @headlessui/react@insiders.

[JesusTheHun]

Hi @RobinMalfait, I tried the new types here, and it seems to me that the type is not correctly set. In the given example, since value is clearly defined as a string | null type, the onChange function should be defined as (v: string | null) => void and not (v: string) => void. Forcing the generic type does not fix the issue.

[RobinMalfait]

Hey @JesusTheHun

I don't think the Combobox is wrong in this case, the actual value has the wrong type. It is currently just string instead of string | null, and therefore the Combobox thinks it is just string as well.

I think this is because strict mode is set to false in your tsconfig.json. Once you set that to true you will see this:

Which results in the correct types.

Even if the type was just string, because of the nullable={true} prop, you will still get string | null in the onChange.

If you set nullable={false} it will be string instead of string | null.

But again, this requires "strict": true otherwise you will see the incorrect types again:

---

Forcing the generic type does not fix the issue.

I think once strict mode is on, that it will be solved.

Without strict mode	With strict mode

[JesusTheHun]

You are right, I didn't add a tsconfig.json in the sandbox, and it works fine in strict mode 👌