Skip to content

ref(core): improve Select typings#110641

Merged
TkDodo merged 17 commits intomasterfrom
tkdodo/ref/improve-select-types
Mar 18, 2026
Merged

ref(core): improve Select typings#110641
TkDodo merged 17 commits intomasterfrom
tkdodo/ref/improve-select-types

Conversation

@TkDodo
Copy link
Copy Markdown
Collaborator

@TkDodo TkDodo commented Mar 13, 2026

The Select component typings were somehow flawed and turn every prop into any, making quite a mess on each call-side because not a single prop on select was properly type-checked.

This PR fixes the select type issues, and in turn revealed over 100 issues on usages. I’ve tried to keep most changes type-only, and I’ll note where we have runtime specific changes in comments.

Most runtime specific changes are about removing props passed to Select that don’t exist and therefore don’t do anything. In many cases, we passed props that are usually passed to a FormField directly to Select.

For some things I’ve just fallen back to as any to not affect anything, which is an okay trade-off because this PR will make all future usages of Select properly typed.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 13, 2026
Comment on lines +468 to +474
/**
* Unlike react-select which expects an OptionType as its value
* we accept the option.value and resolve the option object.
* Because this type is embedded in the OptionType generic we
* can't have a good type here.
*/
value?: any;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll tackle this in a follow-up

Comment on lines +486 to +487
// todo(tkdodo): typing `p` and keeping `any` localized avoids leaking it
const props = p as any;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least this confines the blast radius of any to the inside of this component. It’s an even bigger mess to clean up internally, and I doubt we’ll see it happen. I’d rather throw the select away and do a new one.

export type {
Props,
NamedProps,
NamedProps as Props,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

building on top of NamedProps instead of Props is what enables the type-safety, because for some reason, Props is just NamedProps intersected with:

export type SelectComponentsProps = { [key in string]: any };

🤷‍♂️

}}
options={mapToOptions(emails)}
onBlur={(e: React.ChangeEvent<HTMLInputElement>) => {
onBlur={(e: any) => {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event is a FocusEvent but somehow we expect a ChangeEvent here 🤷

isInsideModal
/>
<TeamSelector
organization={organization}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TeamSelector doesn’t accept organization as a prop - it calls useOrganization() internally.

onChange={({value}: any) => onComparisonDeltaChange(value)}
onChange={({value}) => onComparisonDeltaChange(value)}
options={comparisonDeltaOptions}
required={comparisonType === AlertRuleComparisonType.CHANGE}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required does nothing (I checked)

Comment on lines -129 to +128
options={Object.keys(sortDirections).map(value => ({
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
label: sortDirections[value],
value,
options={Object.entries(sortDirections).map(([value, label]) => ({
value: value as SortDirection,
label,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using Object.entries over Object.keys to avoid the indexed access. value is still a string here so it needs a type assertion.

>
<Select
required
options={METRIC_CHOICES.slice()}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why we made a shallow copy of an object here? I sure don’t hope that the component mutates them, but I’ve inlined nonetheless

options={repositories}
noOptionsMessage={() => t('No repositories found')}
menuPortalTarget={document.body}
prefix={<IconRepository size="sm" />}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no prefix on Select. Maybe this used to be a compactSelect at some point?

value={selectedTemplate}
options={menuOptions}
disabled={disabled}
disabledReason={disabledReason}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another form field level prop, not on Select

@TkDodo TkDodo marked this pull request as ready for review March 13, 2026 18:52
@TkDodo TkDodo requested review from a team as code owners March 13, 2026 18:52
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: New onChange type omits null for clearable selects
    • Updated SingleProps.onChange type to accept OptionType | null to handle react-select passing null when clearable selects are cleared.
  • ✅ Fixed: AsyncProps reintroduces explicitly omitted props in ControlProps
    • Applied Omit to AsyncProps and CreatableProps to prevent reintroduction of onChange, value, menuPlacement, and theme props that were explicitly excluded.

Create PR

Or push these changes by commenting:

@cursor push a33eba44ab
Preview (a33eba44ab)
diff --git a/static/app/components/core/select/select.tsx b/static/app/components/core/select/select.tsx
--- a/static/app/components/core/select/select.tsx
+++ b/static/app/components/core/select/select.tsx
@@ -409,7 +409,7 @@
 
 type SingleProps<OptionType extends OptionTypeBase> = {
   multiple?: false;
-  onChange?: (option: OptionType) => void;
+  onChange?: (option: OptionType | null) => void;
 };
 
 type SelectProps<OptionType extends OptionTypeBase> =
@@ -418,8 +418,8 @@
 
 export type ControlProps<OptionType extends OptionTypeBase = GeneralSelectValue> =
   SelectProps<OptionType> &
-    AsyncProps<OptionType> &
-    CreatableProps<OptionType, boolean> &
+    Omit<AsyncProps<OptionType>, 'onChange' | 'value' | 'menuPlacement' | 'theme'> &
+    Omit<CreatableProps<OptionType, boolean>, 'onChange' | 'value' | 'menuPlacement' | 'theme'> &
     Omit<
       ReactSelectProps<OptionType, boolean>,
       'onChange' | 'value' | 'menuPlacement' | 'theme'

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Copy link
Copy Markdown
Member

@JonasBa JonasBa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A disturbing amount of type issues that we weren't aware of... Having functional types is going to improve the DX by a lot. Judging by usage, I wouldn't be surprised if folks were previously just adding props to see what works 😅

@TkDodo TkDodo merged commit 8fd1c6d into master Mar 18, 2026
63 checks passed
@TkDodo TkDodo deleted the tkdodo/ref/improve-select-types branch March 18, 2026 08:58
JonasBa pushed a commit that referenced this pull request Mar 18, 2026
The Select component typings were somehow flawed and turn every prop
into `any`, making quite a mess on each call-side because not a single
prop on select was properly type-checked.

This PR fixes the select type issues, and in turn revealed over 100
issues on usages. I’ve tried to keep most changes type-only, and I’ll
note where we have runtime specific changes in comments.

Most runtime specific changes are about _removing_ props passed to
`Select` that don’t exist and therefore don’t do anything. In many
cases, we passed props that are usually passed to a `FormField` directly
to Select.

For some things I’ve just fallen back to `as any` to not affect
anything, which is an okay trade-off because this PR will make all
future usages of `Select` properly typed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants