diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx index 0c256d707bc..12b37c9f4bf 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx @@ -633,3 +633,12 @@ describe('Combobox', () => { }); }); }); + +// Type test +(() => { + // Handler function does not allow null for option. + function onChangeHandlerNoNull(option: ComboboxOption) {} + // @ts-expect-error with isClearable set, onChange can pass `null`, so a function that does not accept null + // is an error. If this line errors, then the conditional typing for onChange has been broken. + return ; +})(); diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.tsx index 0d0ab498727..f06ce30060c 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.tsx @@ -20,15 +20,12 @@ import { isNewGroup } from './utils'; // TODO: It would be great if ComboboxOption["label"] was more generic so that if consumers do pass it in (for async), // then the onChange handler emits ComboboxOption with the label as non-undefined. -export interface ComboboxBaseProps + +interface ComboboxStaticProps extends Pick< InputProps, 'placeholder' | 'autoFocus' | 'id' | 'aria-labelledby' | 'disabled' | 'loading' | 'invalid' > { - /** - * An `X` appears in the UI, which clears the input and sets the value to `null`. Do not use if you have no `null` case. - */ - isClearable?: boolean; /** * Allows the user to set a value which is not in the list of options. */ @@ -40,11 +37,6 @@ export interface ComboboxBaseProps */ options: Array> | ((inputValue: string) => Promise>>); - /** - * onChange handler is called with the newly selected option. - */ - onChange: (option: ComboboxOption) => void; - /** * Current selected value. Most consumers should pass a scalar value (string | number). However, sometimes with Async * it may be better to pass in an Option with a label to display. @@ -64,18 +56,32 @@ export interface ComboboxBaseProps onBlur?: () => void; } -type ClearableConditionals = - | { - /** - * Allow the user to clear the selected value. `null` is emitted from the onChange handler - */ - isClearable: true; - /** - * The onChange handler is called with `null` when clearing the Combobox. - */ - onChange: (option: ComboboxOption | null) => void; - } - | { isClearable?: false; onChange: (option: ComboboxOption) => void }; +interface ClearableProps { + /** + * An `X` appears in the UI, which clears the input and sets the value to `null`. Do not use if you have no `null` case. + */ + isClearable: true; + + /** + * onChange handler is called with the newly selected option. + */ + onChange: (option: ComboboxOption | null) => void; +} + +interface NotClearableProps { + /** + * An `X` appears in the UI, which clears the input and sets the value to `null`. Do not use if you have no `null` case. + */ + isClearable?: false; + + /** + * onChange handler is called with the newly selected option. + */ + onChange: (option: ComboboxOption) => void; +} + +export type ComboboxBaseProps = (ClearableProps | NotClearableProps) & + ComboboxStaticProps; export type AutoSizeConditionals = | { @@ -95,9 +101,7 @@ export type AutoSizeConditionals = maxWidth?: never; }; -export type ComboboxProps = ComboboxBaseProps & - AutoSizeConditionals & - ClearableConditionals; +export type ComboboxProps = ComboboxBaseProps & AutoSizeConditionals; const noop = () => {}; @@ -114,7 +118,7 @@ export const Combobox = (props: ComboboxProps) => onChange, value: valueProp, placeholder: placeholderProp, - isClearable = false, + isClearable, // this should be default false, but TS can't infer the conditional type if you do createCustomValue = false, id, width, @@ -244,7 +248,20 @@ export const Combobox = (props: ComboboxProps) => // Instead, stateReducer is called in the same tick as state changes, before that state is committed and rendered. onSelectedItemChange: ({ selectedItem }) => { - onChange(selectedItem); + // `selectedItem` type is `ComboboxOption | null` + // It can be null when `selectItem()` is called with null, and we never do that unless `isClearable` is true. + // So, when `isClearable` is false, `selectedItem` is always non-null. However, the types don't reflect that, + // which is why the conditions are needed. + // + // this is an else if because TS can't infer the correct onChange types from + // (isClearable || selectedItem !== null) + if (isClearable) { + // onChange argument type allows null + onChange(selectedItem); + } else if (selectedItem !== null) { + // onChange argument type *does not* allow null + onChange(selectedItem); + } }, defaultHighlightedIndex: selectedItemIndex ?? 0, diff --git a/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx b/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx index 56a24fd8eef..5fb513f0d7b 100644 --- a/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx @@ -23,9 +23,11 @@ import { MAX_SHOWN_ITEMS, useMeasureMulti } from './useMeasureMulti'; import { useMultiInputAutoSize } from './useMultiInputAutoSize'; import { useOptions } from './useOptions'; -interface MultiComboboxBaseProps extends Omit, 'value' | 'onChange'> { +interface MultiComboboxBaseProps + extends Omit, 'value' | 'onChange' | 'isClearable'> { value?: T[] | Array>; onChange: (option: Array>) => void; + isClearable?: boolean; enableAllOption?: boolean; } diff --git a/yarn.lock b/yarn.lock index cbbffcb3ec9..4e50953f3ed 100644 --- a/yarn.lock +++ b/yarn.lock @@ -14976,8 +14976,8 @@ __metadata: linkType: hard "downshift@npm:^9.0.6": - version: 9.0.8 - resolution: "downshift@npm:9.0.8" + version: 9.0.9 + resolution: "downshift@npm:9.0.9" dependencies: "@babel/runtime": "npm:^7.24.5" compute-scroll-into-view: "npm:^3.1.0" @@ -14986,7 +14986,7 @@ __metadata: tslib: "npm:^2.6.2" peerDependencies: react: ">=16.12.0" - checksum: 10/9dc4577e780c54742ba4dde11f481f0d839f001b309200fbe4db112385b227ccd9cd2ef97d9e995379fa70249f0664a562240e415b9966f18c8a5cb7ce435f2c + checksum: 10/6abc7a585f002f0ebaba1ec42f6102b940257f294d0a33bf189387a316afa24254f0d49c4d5d2553a2263b4e42f497297a8c66dab9bebe24b3dce11fa1456d20 languageName: node linkType: hard