-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Controlled vs Uncontrolled components: how to best represent an "empty but controlled" value for a given prop #47473
Comments
Tagging a few folks who may have insight / opinions on this @diegohaz @sarayourfriend @jsnajdr @tyxla @youknowriad @dmsnell Thank you! 🙏 |
Thanks for reaching out @ciampo. I'm sure I don't properly understand what's happening, other than I think this is another unfortunate example of where the hooks API has made a bit of a mess of things. It sounds like you mainly need a way to determine if the control has been changed at least once? That is, if there's content in an input and we erase that content, giving an empty string, is it still a problem? or is it only on initial load that we don't have a previous value where things get mucky? Remember that we can hold arbitrary values in state and transform them on render. There's a silly trick we can do based on the fact that JS objects are all unique, and that is create something that won't ever be anything else. We can, of course do with with a // set this as the initial value somewhere
const isEmptyToken = {};
<input value={ value === isEmptyToken ? '' : value }> Are we sure that we aren't over-focused on the empty value here and missing a way to reorganize the code so that it's not essential? Would there be another value we create that gets updated by the input's even listener? Is the only thing we need a new latch indicating "yes, we have had at least once change to this input" that gets set on the event listener, which we can then add to the hook's dependencies? |
I'd accept a |
Hey @dmsnell ! Thank you for the quick answer, and excuses for not explaining the problem space clearly.
I'll try to give more context. In the
Given this differentiation, it means that if the Vanilla HTML Now — in our case, we want to apply the controlled/uncontrolled approach to more components, even when they don't necessarily rely on vanilla Hence the main question that was asked at the top of the issue |
Thank you @diegohaz (and everyone else who +1'd). It looks like we may want to go ahead and at least explore this direction |
Hm. I'm not sure apart from I'm still struggling to understand the root problem, so you are welcome to move on without me 😄 Still sounds like part of our conundrum is wanting to use the same value for the state of the component and for the display through the component.
Are we talking about HTML inputs or custom components? Because if React is looking for controlling it then I think we must be discussing HTML inputs, all of which, by the way, only have string types for attributes since all HTML attributes are string, even the boolean ones (and "true" is the empty string to boot!) So again, consider a reframing of the argument. If we're wanting to rewrite our input controls in some new system and want to differentiate "no choice has been made" from "a choice of nothing has been made" then we like need a new token to indicate that, and likely need to intercept that token before rendering it to the final DOM node. This is also another framing of the "Maybe" type, or "Optional" type. Consider passing the value in a list. If the list is empty, it means there's an empty value. If the list has a single value, it's the non-empty value. Lists of more than one item are an error. |
Dennis has a good point, we need to clarify what we're discussing. What I thought I was discussing when I suggested the How we map that "empty-ish" internal state value to the final React element is very straightforward — it depends on the element, but normally we'd just map to an empty string. The reason I'm concerned about using
|
For the external user of the component,
This may be what you're getting at, @mirka, so sorry if I'm just restating what is already obvious. It's probably more work this way, but are there pitfalls in accepting whatever seems reasonable as the "empty" value for the component (whether Apologies if I've misunderstood part of the issue at hand that my question misses. Overall I think |
No pitfalls, just the downside like you mentioned, that it's more complexity to manage in the package as a whole. Keeping it uniform will be simpler for consumers (no need to consult docs) and package maintainers (just use the
We haven't conducted a full audit, but as far as I know the only option is In other words, are there specific reasons why we should prefer |
Isn't the difference that some components support "uncontrolled" behavior and the way to tell it's uncontrolled is by not passing a "value" prop, meaning "value" is |
I think this is the underlying React HTML components ( That doesn't help distinguish how the WordPress component would know that it needs to be uncontrolled though. Is that a problem? Maybe that's what you're getting at, Riad? |
yeah, I'm asking the question about the WP components (controls...). For React, the empty and controlled value is in general "" but our components are not always about strings. |
Our components tend to be used in controlled mode most of the time, so there are likely still some undiscovered bugs related to uncontrolled mode. That said, WP components never really adopted the So already, a good number of WP components don't really distinguish between controlled/uncontrolled modes using an undefined check on |
As we consider using third-party libraries more heavily in the In particular, I looked at Radix UI and ariakit, which are currently the two libraries that we're actively considering. Both libraries have a similar approach:
This is very similar (if not identical) to what we also do in Gutenberg's On top of the "value" and "onChange" props, these libraries also assume a third "defaultValue" prop, which is used to pass an initial value to the component without necessarily "controlling" it. I believe that we should adopt the pattern described above across our components. This also means that "uncontrolled but empty" can literally be any value but |
Relevant conversation happening in #49750 (comment) |
While working on #45771, @mirka @chad1008 and myself started discussing around what is the best way to pass an empty
value
prop to a controlled component:undefined
doesn't seem to be an option, since React associates it to the uncontrolled version of a component. Historically for this use case we've been using empty string (''
), but this approach mostly derives from the fact that vanilla HTML input elements have avalue
prop of typestring
.The point is that an empty string does not represent a good generic solution for when the type of
value
(or any other equivalent prop) is not astring
.We discussed a few approaches, but we feel like we need to explore this topic separately and get to a good solid solution.
Here's a few more considerations that were made:
null
as the "controlled but empty" value since it's type-agnostic, while keepingundefined
to signify uncontrolledThe text was updated successfully, but these errors were encountered: