[BUG] - `Select` TypeScript issues

[paul-vd]

NextUI Version

2.4.0

Describe the bug

Overview

The use of Set for keys in Next-UI components has been confusing and inconsistent, especially when compared to other popular UI libraries like Radix, Material-UI, and AntDesign. Typically, keys should be either a string or an array of strings for multi-selection.

Problem Description

When using TypeScript, the current implementation poses several issues, particularly with the Selection type. The example in the documentation for a controlled component suggests initializing the selection state using useState with a Set:

const [value, setValue] = useState<Selection>(new Set([]));

However, there is confusion regarding the use of Set when referring to the example under "With Error Message" in the documentation. It suggests that the value.has method should be used to check for the presence of a value in the set. However, due to the inclusion of the all string literal in the Selection typing, this results in a TypeScript error:

const [value, setValue] = useState<Selection>(new Set([]));

const isValid = value.has("cat")
                      ~~~ 
//                    Property 'has' does not exist on type 'Selection'.

Impact

This inconsistency makes it challenging to implement a simple Select component without resorting to TypeScript hacks such as type casting. It undermines type safety and leads to confusion about the correct usage of the component.

Suggested Solution

To align with common practices and improve usability, it would be beneficial to:

1. Standardize the type for keys: Use either string or string[] for keys, as is common in other UI libraries.
2. Update the documentation: Ensure that examples and type definitions are clear and consistent, and that they align with the expected usage patterns.
3. Consider removing or rethinking the all string literal: This could prevent conflicts and errors when using TypeScript.

Your Example Website or App

https://stackblitz.com/edit/next-ui-select-typescript?file=src%2Freproduction.tsx&view=editor

Steps to Reproduce the Bug or Issue

1. Create a Controlled Select Component
2. Add the Selection type to the useState
3. Try to use value.has on that selection state

Expected behavior

Should not return a typescript error

Screenshots or Videos

No response

Operating System Version

N/A

Browser

Chrome

[linear[bot]]

ENG-1211 [BUG] - `Select` TypeScript issues

[AnYiEE]

const isValid = value instanceof Set && value.has("cat")

[awesome-pro]

@paul-vd You can simply debug you issue. See the type of Selection is export type Selection = 'all' | Set<Key>

Now you are getting error as TypeScript is trying to ensure that the value variable has the has method, which exists only on Set<Key> and not on the string literal all.

So you can simply avoid the error by:

const [value, setValue] = React.useState<Selection>(new Set(["cat"]));

// Type guard to check if value is a Set
if (value instanceof Set) {
  const isValid = value.has('cat');
  console.log(isValid);
} else {
  // Handle the case where value is 'all'
  console.log("Value is 'all'");
}

If you found you issue resolved kindly close the issue :)

[paul-vd]

@abhinandan-verma sorry but this seems like an overcomplicated "hack" hence my mention of casting (or type checking) and what if you have keys that are string literals for better type safety?

I have updated the example with Material-UI vs NextUI, see how simple it is to implement in MUI without any requirement for hacks vs NextUI which has a confusing Key system which is not really extendable without casting or type inferring.

function MaterialUI() {
  const [value, setValue] = React.useState<Keys>();

  const isValid = value === 'cat';

  return (
    <MuiSelect
      value={value}
      onChange={(event) => setValue(event.target.value)}
      error={isValid}
    >
      {animals.map((animal) => {
        return <MenuItem key={animal.key}>{animal.label}</MenuItem>;
      })}
    </MuiSelect>
  );
}

https://stackblitz.com/edit/next-ui-select-typescript?file=src%2Freproduction.tsx&view=editor

@AnYiEE thanks, but no thanks, this code is a hyper simplification of an actual application, in a real world application, this would require value instanceof Set in multiple locations making the code more complex than what it should actually be.

ps: I also added an example for multi-select with MUI. The DX is 1000% better, which I think is important for a productive UI library.

[awesome-pro]

@paul-vd I understand your point, but the type of Selection is from react-aria side. but I think, for most of the cases, type all has no importance. It can be removed if really not needed, making the uses more easier.

[AnYiEE]

require value instanceof Set in multiple locations

I think you can create a more concise type like:

type YourSelection = Exclude<Selection, 'all'>;
const [value, setValue] = useState<YourSelection>(new Set([]));

Changing existing types (including removing 'all', using arrays instead) is undoubtedly a huge breaking change.

That being said, the performance of Set should be better than arrays, which may also be one of the reasons why NextUI chose Set.

---

Actual use case, reduced a lot of runtime conditional judgments, and still type-safe as before (provided that 'all' will never be used).

[paul-vd]

@AnYiEE see the issue there?

❌ customerStore.shared.beverage.dlcs.set(value as SelectionSet) // casting required

Also, it does not allow strict string literal checks as per my MUI example, something like this would make more sense

 type Keys = "cat" | "dog" | ...
 type TSelection = Set<Keys>

But with both the Exclude or Set<Key>, it makes the onSelectionChange invalid:

onSelectionChange={setValue}
~~~~~~~~~~~~~~~~~
// Type 'Dispatch<SetStateAction<Set<Key>>>' is not assignable to type '(keys: SharedSelection) => void'.

[AnYiEE]

see the issue there?

it makes the onSelectionChange invalid

I can understand what you mean. Because I have restricted the type of value to be within SelectionSet, and my state manager automatically infers that the input type of the setter for value is also SelectionSet. However, the parameter type passed by NextUI's onSelectionChange is always Selection, which is not a subset of SelectionSet.

For this issue, the least disruptive solution I can think of is to dynamically change the parameter type of onSelectionChange based on the types of either defaultSelectedKeys or selectedKeys, similar to how it works with useState. It seems like this would involve a complex type change, but it should be backward compatible.

allow strict string literal checks

Selection does not support this, but I thought about it and feel that it is actually unnecessary. Because value can be used with more precise types, such as type YourSelection = Set<'a' | 'b' | 'c'>, or defining YourSelection as a generic type. Just like mentioned above, Selection is not a subset of YourSelection, and the parameter type of onSelectionChange always remains as Selection, which currently requires using type assertion like so: customerStore.shared.beverage.dlcs.set(value as SelectionSet).

it makes the onSelectionChange invalid

This is also why I didn't change all Selection to SelectionSet (keep), because I want to use it directly like this instead of onSelectionChange={(key) => {customerStore.recipeTableColumns.set(key as SelectionSet)}}. Here, the strict setter parameter type is not significant because the type of selectedKeys is clear, this is a select rather than an input.