Skip to content

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.

Sign up for GitHub

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

Merged
pomfrida merged 1 commit into from
Dec 8, 2025
Merged

fix: update autocomplete types to require optionLabel when using objects and optional for strings/numbers #4279

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions packages/eds-core-react/src/components/Autocomplete/Autocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ type IndexFinderType = <T,>({
allDisabled,
}: {
index: number
optionDisabled: AutocompleteProps<T>['optionDisabled']
availableItems: AutocompleteProps<T>['options']
optionDisabled: (option: T) => boolean
availableItems: readonly T[]
allDisabled?: boolean
calc?: (n: number) => number
}) => number
Expand Down Expand Up @@ -235,7 +235,7 @@ const defaultOptionDisabled = () => false
// MARK: types
export type AutocompleteChanges<T> = { selectedItems: T[] }

export type AutocompleteProps<T> = {
export type AutocompleteProps<T = string> = {
/** List of options in dropdown */
options: readonly T[]
/** Total number of options */
Expand Down Expand Up @@ -293,8 +293,6 @@ export type AutocompleteProps<T> = {
multiple?: boolean
/** Add select-all option. Throws an error if true while multiple = false */
allowSelectAll?: boolean
/** Custom option label. NOTE: This is required when option is an object */
optionLabel?: (option: T) => string
/** Custom option template */
optionComponent?: (option: T, isSelected: boolean) => ReactNode
/** Disable option
Expand Down Expand Up @@ -333,7 +331,21 @@ export type AutocompleteProps<T> = {
* Callback for clear all button
*/
onClear?: () => void
} & HTMLAttributes<HTMLDivElement>
} & HTMLAttributes<HTMLDivElement> &
(T extends string | number
? {
/** Custom option label. NOTE: This is required when option is an object */
Copy link

Copilot AI Nov 28, 2025

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
Suggested change
/** Custom option label. NOTE: This is required when option is an object */
/** Custom option label. NOTE: This is optional for string and number types */

Copilot uses AI. Check for mistakes.
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
})
Comment on lines +337 to +348
Copy link

Copilot AI Nov 28, 2025

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.

Suggested change
/** 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
})

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@FredrikMWold FredrikMWold Dec 1, 2025

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


// MARK: component
function AutocompleteInner<T>(
Expand Down Expand Up @@ -1224,7 +1236,7 @@ function AutocompleteInner<T>(
)
}
// MARK: exported component
export const Autocomplete = forwardRef(AutocompleteInner) as <T>(
export const Autocomplete = forwardRef(AutocompleteInner) as <T = string>(
props: AutocompleteProps<T> & {
ref?: React.ForwardedRef<HTMLInputElement>
/** @ignore */
Expand Down
Loading