Repository navigation
DataViews: vendor ValidatedComboboxControl
#81449
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
Changes from all commits
50faedf
88bab75
295e7a6
ff83cb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,10 @@ | ||
| import { privateApis, Spinner } from '@wordpress/components'; | ||
| import { Spinner } from '@wordpress/components'; | ||
| import { useCallback } from '@wordpress/element'; | ||
| import type { DataFormControlProps } from '../../types'; | ||
| import useElements from '../../hooks/use-elements'; | ||
| import { unlock } from '../../lock-unlock'; | ||
| import { ValidatedComboboxControl } from '../validated-form-controls'; | ||
| import getCustomValidity from './utils/get-custom-validity'; | ||
|
|
||
| const { ValidatedComboboxControl } = unlock( privateApis ); | ||
|
|
||
| export default function Combobox< Item >( { | ||
| data, | ||
| field, | ||
|
|
@@ -19,7 +17,7 @@ export default function Combobox< Item >( { | |
| const value = getValue( { item: data } ) ?? ''; | ||
|
|
||
| const onChangeControl = useCallback( | ||
| ( newValue: string | null ) => | ||
| ( newValue: string | null | undefined ) => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This happened in some of my PRs too: by doing TypeScript playground example: (code)// Minimal reproduction of the ValidatedComboboxControl typing issue
// (Gutenberg PR #81449, packages/dataviews combobox.tsx)
// ─── 1. A trimmed-down ComboboxControlProps ────────────────────────────
// The real one is in packages/components/src/combobox-control/types.ts
type ComboboxControlProps = {
// `value` is an OPTIONAL prop. Optionality adds `undefined` to its type.
value?: string | null;
// `onChange`'s parameter is declared via an indexed access on `value`:
onChange?: ( value: ComboboxControlProps[ 'value' ] ) => void;
};
// Hover this: it's `string | null | undefined`, NOT `string | null`.
type OnChangeParam = ComboboxControlProps[ 'value' ];
declare function ValidatedComboboxControl(
props: ComboboxControlProps
): void;
// ─── 2. Before the PR: the component came from unlock(), which is `any` ─
// Real signature, from @wordpress/private-apis:
// declare function unlock<T = any>(object: unknown): T;
declare function unlock< T = any >( object: unknown ): T;
const UntypedValidatedComboboxControl = unlock( {} ); // type: any
// No type checking at all — the too-narrow handler compiled fine:
UntypedValidatedComboboxControl( {
onChange: ( newValue: string | null ) => {},
} );
// ...and so would something totally wrong:
UntypedValidatedComboboxControl( {
onChange: ( newValue: number ) => {},
} );
// ─── 3. After the PR: the vendored component is properly typed ─────────
// ❌ Error: the component may call onChange(undefined), but this handler
// only accepts string | null. Under strictFunctionTypes a callback must
// accept EVERYTHING the callee might pass (contravariance).
ValidatedComboboxControl( {
onChange: ( newValue: string | null ) => {},
} );
// ✅ This is the fix shipped in the PR:
ValidatedComboboxControl( {
onChange: ( newValue: string | null | undefined ) => {},
} );
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be better to "hoist" the value type, so that we get a narrower and more reliable type in +type ComboboxControlPropsValue = string | null;
type ComboboxControlProps = {
- value?: ComboboxControlProps['value'];
- onChange?: ( value: ComboboxControlProps['value'] ) => void;
+ value?: ComboboxControlPropsValue;
+ onChange?: ( value: ComboboxControlPropsValue ) => void;
};
declare function ValidatedComboboxControl(
props: ComboboxControlProps
): void;
ValidatedComboboxControl( {
- onChange: ( newValue: string | null | undefined ) => {},
+ onChange: ( newValue: string | null ) => {},
} );
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's great feedback, thanks. I've prepared #81568 to address it. Here's a "fun with types" example:
type CaseOne = {
value?: string | null;
onChange?: ( newValue: CaseOne[ 'value' ] ) => void;
};
type CaseTwo = {
value?: string | null;
onChange?( newValue: CaseTwo[ 'value' ] ): void;
}; |
||
| onChange( setValue( { item: data, value: newValue ?? '' } ) ), | ||
| [ data, onChange, setValue ] | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,13 @@ | |
| --wp-components-color-accent: #{$alert-red}; | ||
| border-color: $alert-red; | ||
| } | ||
|
|
||
| // For ComboboxControl | ||
| .components-combobox-control__suggestions-container:has( | ||
| input:invalid[data-validity-visible] | ||
| ):not(:has([aria-expanded="true"])) { | ||
| --wp-admin-theme-color: #{$alert-red}; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
| } | ||
|
|
||
| .dataviews-validated-control__indicator { | ||
|
|
||

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.
I pushed a commit to remove this style as well.