-
Notifications
You must be signed in to change notification settings - Fork 70
fix: update autocomplete types to require optionLabel when using objects and optional for strings/numbers #4279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves type safety for the Autocomplete component by making the optionLabel prop conditionally required based on the option type. When options are objects (anything other than string or number), optionLabel is now required, which aligns with the runtime behavior that throws an error when it's missing. For string or number options, optionLabel remains optional since these primitives can be displayed directly.
Key changes:
- Modified
AutocompletePropstype to use a conditional type that requiresoptionLabelfor object types - Simplified
IndexFinderTypeto use inline types instead of referencingAutocompleteProps
packages/eds-core-react/src/components/Autocomplete/Autocomplete.tsx
Outdated
Show resolved
Hide resolved
packages/eds-core-react/src/components/Autocomplete/Autocomplete.tsx
Outdated
Show resolved
Hide resolved
ff8db8b to
d62f0fa
Compare
…cts and optional for strings/numbers
d62f0fa to
d075c02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
| /** Custom option label. NOTE: This is required when option is an object */ | ||
| optionLabel?: (option: T) => string | ||
| } | ||
| : T extends object | ||
| ? { | ||
| /** Custom option label. NOTE: This is required when option is an object */ | ||
| optionLabel: (option: T) => string | ||
| } | ||
| : { | ||
| /** Custom option label. NOTE: This is required when option is an object */ | ||
| optionLabel?: (option: T) => string | ||
| }) |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The third branch of the conditional type (lines 345-348) appears to be unreachable. In TypeScript, a type T will either extend string | number or object. Types like boolean, undefined, null, symbol, or bigint that don't fit either category are unlikely to be valid option types for an autocomplete component.
Consider simplifying the conditional type to only two branches and removing the unreachable else clause:
} & HTMLAttributes<HTMLDivElement> &
(T extends string | number
? {
/** Custom option label. NOTE: This is optional for primitive types */
optionLabel?: (option: T) => string
}
: {
/** Custom option label. NOTE: This is required when option is an object */
optionLabel: (option: T) => string
})This makes the type definition clearer and removes the dead code path.
| /** Custom option label. NOTE: This is required when option is an object */ | |
| optionLabel?: (option: T) => string | |
| } | |
| : T extends object | |
| ? { | |
| /** Custom option label. NOTE: This is required when option is an object */ | |
| optionLabel: (option: T) => string | |
| } | |
| : { | |
| /** Custom option label. NOTE: This is required when option is an object */ | |
| optionLabel?: (option: T) => string | |
| }) | |
| /** Custom option label. NOTE: This is optional for primitive types */ | |
| optionLabel?: (option: T) => string | |
| } | |
| : { | |
| /** Custom option label. NOTE: This is required when option is an object */ | |
| optionLabel: (option: T) => string | |
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might look like its not need but removing breaks the types when somebody tries to wrap it using the styled from styled components
| } & HTMLAttributes<HTMLDivElement> & | ||
| (T extends string | number | ||
| ? { | ||
| /** Custom option label. NOTE: This is required when option is an object */ |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comments for optionLabel in all three branches are identical: "Custom option label. NOTE: This is required when option is an object". This is misleading because in the first branch (lines 336-339), optionLabel is actually optional for string | number types.
The comment on line 337 should be updated to reflect that it's optional for primitive types:
/** Custom option label. NOTE: This is optional for string and number types */
optionLabel?: (option: T) => string| /** Custom option label. NOTE: This is required when option is an object */ | |
| /** Custom option label. NOTE: This is optional for string and number types */ |
pomfrida
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sublime, thank you ✨
This pull request fixes an issue with the type definition for the
optionLabelprop in the Autocomplete component.Currently,
optionLabelis not marked as required, even when the user provides an array of objects as the options. In those cases, the component throws a runtime error ifoptionLabelis missing.This PR updates the typing so that
optionLabelbecomes required whenever the user supplies an array of objects (anything other thanstring[]ornumber[]).