WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Controlled vs Uncontrolled components: how to best represent an "empty but controlled" value for a given prop #47473

Open ciampo opened 1 year ago

ciampo commented 1 year ago

While working on https://github.com/WordPress/gutenberg/pull/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 a value prop of type string.

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 a string.

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:

ciampo commented 1 year ago

Tagging a few folks who may have insight / opinions on this

@diegohaz @sarayourfriend @jsnajdr @tyxla @youknowriad @dmsnell

Thank you! šŸ™

dmsnell commented 1 year ago

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 Symbol, but we can also do this otherwise.

// 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?

diegohaz commented 1 year ago

I'd accept a null value in this case.

ciampo commented 1 year ago

Hey @dmsnell ! Thank you for the quick answer, and excuses for not explaining the problem space clearly.

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?

I'll try to give more context.

In the @wordpress/components (and generally in Gutenberg) we often deal with "control" (or "input", or "form") components. When dealing with such components, React offers two different ways to deal with their updates:

Given this differentiation, it means that if the value prop of an <input /> isn't passed (or is explicitly passed with a value of undefined), React assumes that the <input /> is uncontrolled. Otherwise, if the <input /> is passed a non-undefined value, React assumes that the <input /> is controlled. React also errors when switching from uncontrolled to controlled.

Vanilla HTML <input />s have a value prop of type string; when the input is "empty", their value is ''. Therefore, when dealing with <input /> in controlled mode, we can use an empty string to set the <input />'s value to an empty state, all without ever switching to uncontrolled mode.

Now ā€” in our case, we want to apply the controlled/uncontrolled approach to more components, even when they don't necessarily rely on vanilla <input />s under the hood. For these components we can't assume that their value is always going to be a string (it could be a number, a custom object, ...)Ā ā€” but at the same time, we want to be able to have a reliable and standardised way to tell those components that their value is "empty" while still in controlled mode.

Hence the main question that was asked at the top of the issue

ciampo commented 1 year ago

I'd accept a null value in this case.

Thank you @diegohaz (and everyone else who +1'd). It looks like we may want to go ahead and at least explore this direction

dmsnell commented 1 year ago

Hm. I'm not sure apart from null/undefined if it will be easy to find a universal "empty" value. Even null sadly can be a affirmatively chosen value.

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.

For these components we can't assume that their value is always going to be a string (it could be a number, a custom object, ...)

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.

mirka commented 1 year ago

Dennis has a good point, we need to clarify what we're discussing.

What I thought I was discussing when I suggested the usePrevious is how we were going to improve the logic of our useControlledValue hook. This is only about how we negotiate the consumer-provided value with internal state, and has nothing to do with what React considers as an uncontrolled value on form elements. It's specifically how our useControlledValue hook itself decides whether the value is controlled or not.

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 null is:

sarayourfriend commented 1 year ago

For the external user of the component, undefined, null, whatever makes sense for the type of the input makes sense (preferring null over undefined, but that's just my taste).

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.

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 '' or null, etc) and mapping it to something internally that will work for the underlying type of component? Are the interfaces too generic for that approach?

Apologies if I've misunderstood part of the issue at hand that my question misses.

Overall I think null is a good ideal choice. If it's not possible, I wonder what options wouldn't have the backwards compatibility and type issues that @mirka points out. If backwards compatibility is important to keep in mind (which it tends to be, if my memory serves, with these packages), enumerating the choices that do not have the issue would make it easier to choose between them.

mirka commented 1 year ago

It's probably more work this way, but are there pitfalls in accepting whatever seems reasonable as the "empty" value for the component

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 useControlledValue hook).

I wonder what optionsĀ wouldn'tĀ have the backwards compatibility and type issues

We haven't conducted a full audit, but as far as I know the only option is undefined. I'm personally hoping we can keep using undefined if possible (with the usePrevious approach or whatever else), as this is pretty well aligned with the mental model of how we generally use controls in the Gutenberg editor, especially in the Tools Panel paradigm. Control values tend to start out as undefined, and can also be reset back to an undefined state.

In other words, are there specific reasons why we should prefer null over undefined?

youknowriad commented 1 year ago

In other words, are there specific reasons why we should prefer null over undefined?

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 undefined while passing null indicates that it's controlled but empty.

sarayourfriend commented 1 year ago

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 undefined while passing null indicates that it's controlled but empty.

I think this is the underlying React HTML components (<input />). Based on what @mirka is saying here, in response to my question about "intercepting" the value, the WordPress component could accept undefined and use that as a signal to pass whatever the appropriate "empty" value is for the specific HTML component would be.

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?

youknowriad commented 1 year ago

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.

mirka commented 1 year ago

That doesn't help distinguish how theĀ WordPress componentĀ would know that it needs to beĀ uncontrolledĀ though. Is that a problem?

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 defaultValue convention that is common in other component libraries, where you specifically use this prop instead of value when you want to set a default value in uncontrolled mode. This means that for WP components, you generally can just pass a (non-empty) default value to the value prop even if you're going to use it in uncontrolled mode.

So already, a good number of WP components don't really distinguish between controlled/uncontrolled modes using an undefined check on value. They just have their own hook logic to negotiate the incoming values with internal state. And there is at least one component (ToggleGroupControl) that already uses the usePrevious pattern to do this, so I'm hoping this is something we can bake into useControlledState for easy reuse.

ciampo commented 1 year ago

As we consider using third-party libraries more heavily in the @wordpress/components package, I took a look at what these libraries do when handling controlled/uncontrolled components.

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 useControlledValue hook

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 undefined, and it's probably down to the specific component to define what that should be. For example, input-based components could use an empty string. Components accepting an array of values could use an empty array, etc etc.

ciampo commented 1 year ago

Relevant conversation happening in https://github.com/WordPress/gutenberg/pull/49750#issuecomment-1508577543