ref(scraps): make value and onChange required on compactSelect #103654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
since we're switching towards fully controlled usages now that we don't have `defaultValue` anymore, it makes sense to require this on type-level. Most violations were in stories and tests, and in those cases where the value is actually irrelevant e.g. because we do a navigation, we can still explicitly pass value={undefined}
TkDodo
commented
Nov 19, 2025
Comment on lines
-137
to
-140
| <VisibleFocusButton | ||
| <Button | ||
| size="xs" | ||
| borderless | ||
| style={{background: filters.length > 0 ? theme.purple100 : 'transparent'}} |
Collaborator
Author
There was a problem hiding this comment.
note: this didn’t work with UI2 at all so I’ve just removed it an use a regular button now. looks fine to me:
| if (isError) { | ||
| return ( | ||
| <Tooltip title={t('Error loading span categories')}> | ||
| <CompactSelect disabled options={[]} value={undefined} onChange={onChange} /> |
Collaborator
Author
There was a problem hiding this comment.
note: this should probably just be a Button but 🤷♂️
JonasBa
reviewed
Nov 19, 2025
| ); | ||
| } | ||
|
|
||
| const VisibleFocusButton = styled(Button)` |
JonasBa
reviewed
Nov 19, 2025
JonasBa
approved these changes
Nov 19, 2025
| <CompactSelect | ||
| grid | ||
| value={undefined} | ||
| onChange={jest.fn} |
Contributor
There was a problem hiding this comment.
Bug: CompactSelect test mocks created incorrectly without function call
All onChange mock assignments use jest.fn (without parentheses) instead of jest.fn() (with parentheses). This passes the jest factory function reference rather than a mock function instance, preventing call tracking. The correct pattern onChange={jest.fn()} is used in composite.spec.tsx and throughout the codebase.
TkDodo
added a commit
that referenced
this pull request
Nov 24, 2025
This PR generally aims to improve performance of the `compactSelect` component by _not_ rendering all options eagerly. This is achieved with a simple conditional rendering, however, some pre-requisites are required to get that going. > Why did we render all those options even though the select was closed? Good question! We did that because the `compactSelect` component internally holds a `<List>`, which managed its state internally. That state is (was) bubbled up to the parent (with some effects, inverting the parent-child data flow into child-parent), so that the parent could create the `label` of the select trigger. For that, it needed to render all children, otherwise the bubble-up didn’t work. This just shows why child->parent dataflows are bad design. We need to render e.g. 1000 options just to get to the correct label of a button. It’s not react. Furthermore, the whole internal state of `<List>` is unnecessary, because we use the component in a fully controlled way exclusively. The parent has the state already! Another reminder that state duplication & syncing is the root of all evil. So now, we’re using the parent state to calculate that and remove the internal state. That means we can also conditionally render options only when the select is opened. Getting there was painful: First, we needed to make sure that we disallow uncontrolled usage of this component: - #103637 And make it required that we pass `value` and `onChange`: - #103654 Then, there’s `CompositeSelect`, which shares some internals with `CompactSelect`, however for those, the parent doesn’t pass the full state to `CompositeSelect` itself because it’s built up of `Regions`. The good news is that we never use the default trigger label, so we don’t need to calculate it based on the current selection. To make sure we don’t do this in the future, `CompositeSelect` now has a required trigger: - #103713 Lastly, there’s `clearable`. The `Clear` button appears _outside_ of `List`, so it doesn’t have all the information necessary to perform a clear if we remove the reverse data flow. I changed how `clear` works by just triggering an `onChange` with an empty value. Also, types were a mess and that’s now fixed: - #103720 With that, I could finally remove the internal `useLayoutEffect` that synced state from the `List` child with the `Control` parent. ### Test Changes Then, many tests failed. Note that I checked all failing tests in the product before I changed them, and they all work fine. A couple of thoughts on why the tests failed: - They sometimes relied on the internal state management and mocked `onChange`. Now, without `onChange`, there is no state change happening. I’ve changed some tests to do some basic `useState` around the component (like we do in the product, too). - They sometimes had tests that relied upon all option popups being rendered, so they were accessing the Nth search field. Now they are no longer in the DOM because of conditional rendering so all of those needed to update.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
since we're switching towards fully controlled usages now that we don't have
defaultValueanymore, it makes sense to require this on type-level. Most violations were in stories and tests, and in those cases where the value is actually irrelevant e.g. because we do a navigation, we can still explicitly pass value={undefined}