feat(Select): support generic value types#2727
Open
Nowely wants to merge 10 commits into
Open
Conversation
Contributor
Reviewer's GuideExtends the Select component and its hooks to support generic, non-string value types (numbers and objects) end-to-end, updates utilities, hidden form behavior, docs and stories accordingly, and adds comprehensive runtime and type-level tests plus a couple of selection semantics fixes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
added 8 commits
July 4, 2026 14:33
…logic - Updated `FlattenOption` type to use generic types for better flexibility. - Enhanced `serializeOptionValue` to convert non-finite numbers to strings. - Adjusted `getSelectedOptionsContent` to handle NaN and Infinity values correctly. - Modified `SelectControl` to accurately determine if a value is present. - Refined selection logic in `useSelect` to correctly compare values, including NaN.
…on logic - Removed outdated implementation plan and design spec files. - Introduced `getOptionValueKey` to manage object value keys by reference. - Updated `serializeOptionValue` to handle various value types, including NaN. - Enhanced tests for handling numeric values and NaN in selection scenarios. - Ensured backward compatibility while supporting generic value types in Select component.
… value handling - Added tests for handling options with children and text properties in getSelectedOptionsContent. - Improved coverage for various value types including boolean, null, and undefined. - Updated existing tests to reflect changes in option presence logic and selection behavior. - Refined test descriptions for clarity and consistency.
- Clarified descriptions regarding the use of generic value types in the Select component. - Improved explanations on how non-string values are handled, including serialization in forms. - Enhanced consistency in terminology and formatting across both language versions. - Removed outdated or redundant information to streamline the documentation.
… related components
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- Selection logic in OptionWrap still uses
value.includes(option.value), which diverges from the SameValueZero semantics introduced inuseSelect(e.g. NaN handling); consider reusing the same comparison helper there to keep UI selection state consistent. - HiddenSelect and getSelectedOptionsContent now rely on
serializeOptionValuefor non-string values; if you expect very large or complex objects, you might want to document or constrain this behavior to avoid surprising JSON outputs or performance hits when many values are serialized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Selection logic in OptionWrap still uses `value.includes(option.value)`, which diverges from the SameValueZero semantics introduced in `useSelect` (e.g. NaN handling); consider reusing the same comparison helper there to keep UI selection state consistent.
- HiddenSelect and getSelectedOptionsContent now rely on `serializeOptionValue` for non-string values; if you expect very large or complex objects, you might want to document or constrain this behavior to avoid surprising JSON outputs or performance hits when many values are serialized.
## Individual Comments
### Comment 1
<location path="src/components/Select/__tests__/Select.generic-value.test.tsx" line_range="232-241" />
<code_context>
+ it('serializes non-string values in forms', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for multiple generic values being submitted in forms
Since `HiddenSelect` generates multiple hidden inputs for multi-select, this test should also cover a `multiple` scenario with two non-string values (e.g., two numbers or two objects) selected and submitted. Verify that the `FormData` contains one entry per selected value and that each is serialized per the documented rules (`String` for numbers, `JSON.stringify` for objects), so multi-select submissions with generic values are properly exercised.
Suggested implementation:
```typescript
it('serializes non-string values in forms', async () => {
const item = {id: 1, label: 'First'};
const secondItem = {id: 2, label: 'Second'};
const formDataRef: {current: FormData | null} = {current: null};
const multiFormDataRef: {current: FormData | null} = {current: null};
render(
<>
{/* single-value form (existing coverage) */}
<form
data-testid="single-value-form"
onSubmit={(e) => {
e.preventDefault();
formDataRef.current = new FormData(e.currentTarget);
}}
>
{/* existing single-value Select / controls remain below */}
</form>
{/* multi-select form with multiple generic values */}
<form
data-testid="multi-value-form"
onSubmit={(e) => {
e.preventDefault();
multiFormDataRef.current = new FormData(e.currentTarget);
}}
>
{/* HiddenSelect should render multiple hidden inputs for a multi-select,
each with the same name and a serialized value. This Select instance
is configured to exercise that behavior with two non-string values. */}
<Select
name="generic-multi-select"
// two non-string values (objects) selected
value={[item, secondItem]}
// multi-select mode so HiddenSelect generates one hidden input per value
isMulti
options={[item, secondItem]}
/>
<button type="submit">Submit multi</button>
</form>
</>
```
To fully implement the suggested coverage:
1. Ensure the `Select` component in this test file accepts `name`, `value`, `isMulti`, and `options` props as used above. If your API differs (e.g., `multiple` instead of `isMulti`, or `selected` instead of `value`), adjust the props accordingly.
2. After the existing interactions for the single-value form, add a `userEvent.click` (or equivalent) on the "Submit multi" button and assertions like:
- `expect(multiFormDataRef.current).not.toBeNull();`
- `const values = multiFormDataRef.current!.getAll('generic-multi-select');`
- `expect(values).toHaveLength(2);`
- `expect(values[0]).toBe(JSON.stringify(item));`
- `expect(values[1]).toBe(JSON.stringify(secondItem));`
3. If you also want to cover numeric generic values in the multi-select scenario, add another multi-select instance (or extend the existing one) with numeric options and assert that the corresponding `FormData` entries are plain strings (e.g., `'1'`, `'2'`) rather than JSON.
4. Remove or refactor the temporary empty `<form>` wrapper comment `"existing single-value Select / controls remain below"` so the original single-value Select JSX stays in its existing form section, and the new multi-select JSX stays in the second form section.
</issue_to_address>
### Comment 2
<location path="src/components/Select/__tests__/Select.generic-value.test.tsx" line_range="380-389" />
<code_context>
+ expect(onUpdate).toHaveBeenCalledWith([]);
+ });
+
+ it('renders a NaN option as selected in multiple mode', async () => {
+ render(
+ <Select
+ qa="select"
+ multiple
+ value={[NaN]}
+ options={[
+ {value: NaN, content: 'NaN'},
+ {value: 1, content: 'One'},
+ ]}
+ />,
+ );
+
+ await user.click(screen.getByTestId('select'));
+
+ const shownClass = getTickIconShownClass();
+ // eslint-disable-next-line testing-library/no-node-access
+ const nanTick = screen.getByRole('option', {name: 'NaN'}).querySelector(`.${shownClass}`);
+ // eslint-disable-next-line testing-library/no-node-access
+ const oneTick = screen.getByRole('option', {name: 'One'}).querySelector(`.${shownClass}`);
+
+ expect(nanTick).not.toBeNull();
+ expect(oneTick).toBeNull();
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for SameValueZero comparison in single-select mode (including NaN)
The current tests validate NaN handling only in multiple-select mode (deselection and visual selection), but SameValueZero semantics must also hold in single-select. Please add a single-select test where:
- `Select` is rendered with `multiple={false}`, `value={[NaN]}`, and an option `{value: NaN, content: 'NaN'}`.
- The NaN option is clicked again.
- `onUpdate` is asserted not to be called.
This will verify that single-select re-selection of NaN is treated as the same value and does not trigger updates, matching the SameValueZero behavior described in the PR.
</issue_to_address>
### Comment 3
<location path="src/components/Select/utils.tsx" line_range="24" />
<code_context>
export type GroupTitleItem<T = any> = {label: string; disabled: true; data?: T};
-export type FlattenOption = SelectOption | GroupTitleItem;
+export type FlattenOption = SelectOption<any, any> | GroupTitleItem;
export type FlattenOptions = FlattenOption[] & {
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing local type aliases for the generic Select types and collapsing the option value key helpers into a single function to keep the new behavior while reducing cognitive and visual complexity in this file.
The generics + key helpers are adding noticeable complexity. You can keep the new functionality but reduce cognitive load with two targeted refactors:
---
### 1. Introduce internal type aliases
Most of the noise is from repeating `SelectOption<unknown, unknown>` / `SelectProps<unknown, unknown>` etc. Local aliases keep external typings generic while making this file easier to read and maintain.
```ts
// Near the top of the file
type AnySelectProps = SelectProps<unknown, unknown>;
type AnySelectOption = SelectOption<unknown, unknown>;
type AnySelectOptionGroup = SelectOptionGroup<unknown, unknown>;
type AnySelectOptions = SelectOptions<unknown, unknown>;
```
Then simplify usages:
```ts
export type FlattenOption = AnySelectOption | GroupTitleItem;
export const isSelectGroupTitle = (
option?: AnySelectOption | AnySelectOptionGroup,
): option is GroupTitleItem => {
return Boolean(option && 'label' in option);
};
export const getFlattenOptions = (options: AnySelectOptions): FlattenOptions => {
// ...
};
const getOptionText = (option: AnySelectOption): string => {
// ...
};
export const getSelectedOptionsContent = (
options: AnySelectOptions,
value: unknown[],
renderSelectedOption?: AnySelectProps['renderSelectedOption'],
): React.ReactNode => {
const flattenSimpleOptions = options.filter((opt) => !isSelectGroupTitle(opt)) as AnySelectOption[];
const optionsMap = new Map<unknown, AnySelectOption>(
flattenSimpleOptions.map((opt) => [opt.value, opt]),
);
// ...
};
const getTypedChildrenArray = (children: AnySelectProps['children']) => {
return React.Children.toArray(children) as (
| React.ReactElement<AnySelectOption, typeof Option>
| React.ReactElement<AnySelectOptionGroup, typeof OptionGroup>
)[];
};
const getOptionsFromOptgroupChildren = (
children: AnySelectOptionGroup['children'],
) => {
return (
React.Children.toArray(children) as React.ReactElement<AnySelectOption, typeof Option>[]
).reduce(
(acc, {props}) => {
if ('value' in props) {
acc.push(props);
}
return acc;
},
[] as AnySelectOption[],
);
};
export const getOptionsFromChildren = (children: AnySelectProps['children']) => {
return getTypedChildrenArray(children).reduce(
(acc, {props}) => {
// ...
},
[] as (AnySelectOption | AnySelectOptionGroup)[],
);
};
const isOptionMatchedByFilter = (option: AnySelectOption, filter: string) => {
// ...
};
export const getFilteredFlattenOptions = (args: {
options: FlattenOption[];
filter: string;
filterOption?: AnySelectProps['filterOption'];
}) => {
// ...
};
```
Actionable outcome: same type safety, but much less visual clutter.
---
### 2. Flatten `serializeOptionValue` / `getOptionValueKey`
Right now there’s a layered abstraction:
- `getOptionValueKey` does identity-based keys for objects and delegates primitives to…
- `serializeOptionValue`, which has a `JSON.stringify` + `try/catch` branch.
You can keep the same semantics (identity for objects, string conversion for primitives) but make it more obvious by collapsing this into a single, documented helper.
```ts
// Replace serializeOptionValue + getOptionValueKey with:
export const getOptionValueKey = (() => {
const objectKeys = new WeakMap<object, string>();
let nextId = 0;
/**
* Stable React key for option values:
* - primitives: String(value)
* - objects: identity-based id, stable for the life of the object
*/
return (value: unknown): string => {
if (typeof value === 'object' && value !== null) {
let key = objectKeys.get(value);
if (!key) {
key = `obj-${nextId++}`;
objectKeys.set(value, key);
}
return key;
}
// primitives (string, number, boolean, symbol, bigint, undefined, null)
return String(value);
};
})();
```
Then simplify usage:
```ts
const getOptionText = (option: AnySelectOption): string => {
if (typeof option.content === 'string') {
return option.content;
}
if (typeof option.children === 'string') {
return option.children;
}
if (option.text) {
return option.text;
}
return String(option.value);
};
export const getSelectedOptionsContent = (
options: AnySelectOptions,
value: unknown[],
renderSelectedOption?: AnySelectProps['renderSelectedOption'],
): React.ReactNode => {
// ...
if (renderSelectedOption) {
return selectedOptions.map((option, index) => (
<React.Fragment key={getOptionValueKey(option.value)}>
{renderSelectedOption(option, index)}
</React.Fragment>
));
}
// ...
};
```
This keeps:
- identity-based keys for object values (same as current behavior with `WeakMap`),
- predictable string keys for primitives,
but removes the extra indirection and `JSON.stringify` complexity, making the key strategy easier to understand and maintain.
</issue_to_address>
### Comment 4
<location path="src/hooks/useSelect/useSelect.ts" line_range="8" />
<code_context>
import {useOpenState} from './useOpenState';
-export const useSelect = <T extends unknown>({
+export function useSelect<T extends unknown, V = string>(
+ props: UseSelectProps<V>,
+): UseSelectResult<T, V>;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the hook’s typing by removing overloads, limiting generic propagation, and tightening the equality helper to keep SameValueZero behavior with less indirection.
The overloads and pervasive generics do increase complexity without changing core behavior. You can keep the new value typing while simplifying the public API and internal implementation.
### 1. Drop overloads, keep a single generic signature
You can keep `V` support and still have good inference with a single generic signature; callers that care about `V` can specify it explicitly, others get `string`:
```ts
// Before
export function useSelect<T extends unknown, V = string>(
props: UseSelectProps<V>,
): UseSelectResult<T, V>;
export function useSelect(props: UseSelectProps): UseSelectResult<unknown>;
export function useSelect<T extends unknown, V = string>({
// ...
}: UseSelectProps<V>): UseSelectResult<T, V> { /* ... */ }
// After
export function useSelect<T extends unknown, V = string>({
// ...
}: UseSelectProps<V>): UseSelectResult<T, V> {
// implementation unchanged
}
```
This removes the mental overhead of overload resolution and the comment about `ReturnType/Parameters`, while preserving the new generic result type.
### 2. Avoid propagating generics through all internal types
You can keep `UseSelectResult<T, V>` unchanged but simplify internal types by aliasing the option type:
```ts
// Before
const handleSingleSelection = React.useCallback(
(option: UseSelectOption<T, V>) => { /* ... */ },
[value, setValue, toggleOpen],
);
const handleMultipleSelection = React.useCallback(
(option: UseSelectOption<T, V>) => { /* ... */ },
[value, setValue],
);
// After
type InternalOption<V> = UseSelectOption<unknown, V>;
const handleSingleSelection = React.useCallback(
(option: InternalOption<V>) => { /* ... */ },
[value, setValue, toggleOpen],
);
const handleMultipleSelection = React.useCallback(
(option: InternalOption<V>) => { /* ... */ },
[value, setValue],
);
```
This keeps external type parameters but reduces the noise of `<T, V>` everywhere in the implementation.
### 3. Inline or simplify equality helper while retaining SameValueZero
If the SameValueZero semantics are required, you can keep them but reduce indirection by inlining the helper or giving it a tighter signature:
```ts
// Before
function isSameValue(a: unknown, b: unknown) {
return (
a === b ||
(typeof a === 'number' && typeof b === 'number' && Number.isNaN(a) && Number.isNaN(b))
);
}
const handleSingleSelection = React.useCallback(
(option: UseSelectOption<T, V>) => {
if (!value.some((v) => isSameValue(v, option.value))) {
// ...
}
},
[value, setValue, toggleOpen],
);
// After (same behavior, less indirection)
const sameValueZero = (a: V, b: V) =>
a === b ||
(typeof a === 'number' && typeof b === 'number' && Number.isNaN(a) && Number.isNaN(b));
const handleSingleSelection = React.useCallback(
(option: InternalOption<V>) => {
if (!value.some((v) => sameValueZero(v, option.value))) {
const nextValue = [option.value];
setValue(nextValue);
}
toggleOpen(false);
},
[value, setValue, toggleOpen],
);
```
Alternatively, if SameValueZero isn’t strictly needed for this hook, you can use `Object.is` (with a short comment) and restore the simpler includes/filter semantics:
```ts
const equalsValue = (a: V, b: V) => Object.is(a, b); // NaN equals NaN, -0/+0 differ
const handleMultipleSelection = React.useCallback(
(option: InternalOption<V>) => {
const alreadySelected = value.some((v) => equalsValue(v, option.value));
const nextValue = alreadySelected
? value.filter((iteratedVal) => !equalsValue(iteratedVal, option.value))
: [...value, option.value];
setValue(nextValue);
},
[value, setValue],
);
```
These changes keep the new generic value support and equality behavior but reduce the surface area of overloads and generics, making the hook easier to read and maintain.
</issue_to_address>
### Comment 5
<location path="src/components/Select/Select.tsx" line_range="60" />
<code_context>
- (option?: FlattenOption) => {
- if (!option || option?.disabled || 'label' in option) {
- return;
+export const Select = React.forwardRef<HTMLButtonElement, SelectProps<unknown, unknown>>(
+ function Select(rawProps: SelectProps<unknown, unknown>, ref: React.Ref<HTMLButtonElement>) {
+ const props = useDefaultProps('Select', rawProps);
</code_context>
<issue_to_address>
**issue (complexity):** Consider separating the internal Select implementation from its public generic typing to avoid `unknown`-based props, dual call signatures, and the `unknown as SelectComponent` cast.
The added generics are valuable, but the current typing of `Select` does introduce avoidable complexity (`unknown` props + dual call signatures + `unknown as SelectComponent` cast).
You can keep all functionality while simplifying the types by:
### 1. Use a clear internal props alias instead of `<unknown, unknown>`
This makes the intent obvious and avoids the confusing `unknown` usage:
```ts
type AnySelectProps = SelectProps<any, any>;
const SelectInner = React.forwardRef<HTMLButtonElement, AnySelectProps>(
function SelectInner(rawProps: AnySelectProps, ref: React.Ref<HTMLButtonElement>) {
const props = useDefaultProps('Select', rawProps);
// ... existing implementation body unchanged ...
},
);
```
### 2. Keep the external generic surface, but separate it from the internal implementation
Instead of casting the `forwardRef` result directly as `unknown as SelectComponent`, define `SelectComponent` only for the public API and assign the internal implementation to it:
```ts
type SelectComponent = {
// Generic external signature
<T = any, V = string>(
p: SelectProps<T, V> & {ref?: React.Ref<HTMLButtonElement>}
): React.ReactElement;
// Optional “compat” signature for Storybook/React.ComponentProps, if still needed
(p: SelectProps & {ref?: React.Ref<HTMLButtonElement>}): React.ReactElement;
Option: typeof Option;
OptionGroup: typeof OptionGroup;
};
export const Select = SelectInner as SelectComponent;
Select.Option = Option;
Select.OptionGroup = OptionGroup;
```
This keeps:
- Runtime implementation typed with a single, straightforward `AnySelectProps`.
- Public API generic over `<T, V>` as before.
- Storybook/`React.ComponentProps` compatibility via the overload, but without pushing that workaround into the `forwardRef` call or using `unknown` everywhere.
If you later find you can drop the second call signature (e.g., by introducing a separate helper type for Storybook), you can simplify `SelectComponent` to a single generic signature without touching the implementation body.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Preview is ready. |
Nowely
force-pushed
the
feat/ass-select-generic-value
branch
from
July 4, 2026 11:34
eeb1368 to
7c9e171
Compare
Contributor
|
🎭 Component Tests Report is ready. |
Contributor
Author
There's no divergence -
That's already in the README: numbers go through |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(Select): support generic value types
value,defaultValue,onUpdateandoptions[].valueare now generic over a trailingV = stringparameter — numbers and objects work directly, inferred from props:All option callbacks (
renderOption,filterOption,getOptionHeight, …) receive the typed value. The<Select.Option>children API can't infer non-string types — explicit type arguments, documented in the README.Semantics
valueentry equalsoption.valuethe wayArray.includescompares (SameValueZero): strings and numbers match by value (NaNmatchesNaNtoo), objects match only if they are the same instance. Structurally equal copies ({id: 1}vs another{id: 1}) do NOT match — pass the same object references invaluethat you used inoptions.content/textas usual; if an option has none, the serialized value is used (42,{"id":1}). Previously a non-string value withoutcontentcrashed thefilterablepath.nameprop)? Strings as is, numbers viaString(value), objects viaJSON.stringify(value). Form reset restores the original values, references included.Backward compatibility
= stringdefaults — existing annotations keep their meaning.React.ComponentProps/ReturnType/StorybookMetaignore generic defaults, soSelect,Select.Option/OptionGroupanduseSelectgot non-generic trailing overloads to keep resolving to string values. Pinned by type tests.ControlGroupOptionuntouched:SelectOptionredeclaresvalue: V.'__SELECT_LIST_ITEM_LOADING__'used to render as a loading spinner (collision with an internal sentinel string). The sentinel is now matched by object identity, so no user value can collide with it.value={[0]}styled the control as empty (the old check wasfilter(Boolean), and0is falsy). Now only'',nullandundefinedcount as empty.Summary by Sourcery
Add generic value type support to the Select component while preserving backward-compatible string defaults and improving value handling semantics.
New Features:
Bug Fixes:
Enhancements:
Tests: