ref: do not allow uncontrolled compactSelect #103637
Merged
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
gggritso
approved these changes
Nov 19, 2025
isabellaenriquez
approved these changes
Nov 19, 2025
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.
defaultValueis used for uncontrolled components - those that do not have anonChangehandler. You usually either passdefaultValue(fully uncontrolled), or you passvalue+onChange(fully controlled)uncontrolled only makes sense in forms where you take the “html” value from the form submit handler, which is not something we do right now. All usages except one in production are controlled usages.
The one usage in
gsApp/views/subscriptionPage/usageLog.tsxpasses adefaultValuedecoded from the url and has anonChangehandler that navigates. This will work just the same when passingvalue. Actually usingnuqswould make this explicit.Note: This is a preparation for a larger refactoring / perf improvement of
compactSelectto avoid rendering all options eagerly.