Skip to content
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

Textarea does not respond to the value property changing to null #2533

Closed
rszewczyk opened this issue Nov 16, 2014 · 64 comments
Closed

Textarea does not respond to the value property changing to null #2533

rszewczyk opened this issue Nov 16, 2014 · 64 comments

Comments

@rszewczyk
Copy link

When the value property of a textarea changes to null or undefined from a non blank value, the text displayed in the DOM does not change. Here is a fiddle that captures the issue - notice that behavior is different for the text input.

http://jsfiddle.net/f1v1fac1/

@syranide
Copy link
Contributor

I believe it's input that is misbehaving here, not textarea. Setting value to null makes it uncontrolled and it should make no attempt to change the value.

@rszewczyk
Copy link
Author

Here's the case I'm thinking of: Imagine a series of state transitions (perhaps instead of a single button the form would have some sort of selector that loads a series of objects from a data source). If one of those transitions sets a property to null, you end up with a value from the last object in the textarea. Doesn't seem right.

I can see though what you're saying. I might be taking the concept of two way binding a little too literally - I'm currently working around it by doing something like this:

http://jsfiddle.net/f1v1fac1/2/

...which isn't a big deal.

@zpao
Copy link
Member

zpao commented Nov 19, 2014

@syranide does setting to null make it uncontrolled? That doesn't sound right to me, but I haven't looked at the linkedstatemixin in a long time. I also would expect the behavior of the input here, not the textarea.

@syranide
Copy link
Contributor

@zpao http://jsfiddle.net/91khmysc/ null is treated as (mostly) identical to undefined (or not set) through-out React I think?

Also yeah I agree, the more I think about it more it makes sense to "reset" it when becoming uncontrolled, it's the "most deterministic" one and also the one easiest to reason about.

@rymohr
Copy link

rymohr commented Nov 19, 2014

This one was driving me nuts too until I finally got down to the bottom of things. Here's a simpler example that doesn't use valueLink.

http://jsfiddle.net/wn7ymsa4/3/

Pretty sure if a value/valueLink/checked/checkedLink property was given the component should be controlled, regardless of the value assigned.

@martinstein
Copy link

The same problem also occurs for a <select> value. I've extended the jsfiddle to show the select-problem, too:
http://jsfiddle.net/27bqLvLL/1/

@martinstein
Copy link

Okay, I think my pull request above #3041 should fix the issue and have correct logic (so this one should be preferred over the pull request #2534).

If I get the heads up, I have a similar fix for the same problem for <select> components.

@rymohr
Copy link

rymohr commented Feb 5, 2015

I still think react should be checking for the presence of a value key, not the value itself. If you want the component to be uncontrolled, don't pass a value prop.

// controlled
<textarea value={this.props.text} />

// uncontrolled
<textarea />

Removes any possible confusion since you no longer need to know all possible values of this.props.text to know if the textarea is controlled or not.

@martinstein
Copy link

I don't disagree, but I think your line of thinking is a bigger change (that would also affect the documentation). So I'd suggest to go with the pull request #3041 for now.

But for the overall picture, I think you're right. There should be a clearer distinction between controlled/uncontrolled behavior, that avoids edge cases like these:

http://facebook.github.io/react/tips/controlled-input-null-value.html

@syranide
Copy link
Contributor

syranide commented Feb 5, 2015

@rymohr While it makes sense in a way, it breaks with how all other props work and I feel like there's something inherently wrong about <textarea value={undefined} /> being a controlled component as well. While I don't have this on authority, I'm pretty sure that JS defines undefined as meaning to be literally undefined, treating it as anything else breaks expectations.

@rymohr
Copy link

rymohr commented Feb 5, 2015

True, but this is about developer intention and "falling into the pit of success" as you guys like to say. It happens with null values too, not just undefined.

Nobody passes a value prop to a component they want to be uncontrolled. They use defaultValue and placeholder.

Switching to a key-presence check would simplify the underlying code and make controlled vs uncontrolled components easier to explain. It's easy to misinterpret the current documentation:

An with value set is a controlled component.

In the examples above, value is set, it just happens to be set to null.

@rymohr
Copy link

rymohr commented Feb 5, 2015

I believe it's input that is misbehaving here, not textarea @syranide

It should also be revealing that nobody's complained about the current behavior of input -- it's the textarea behavior people find surprising (regardless of which is technically misbehaving by react expectations).

@zpao
Copy link
Member

zpao commented Feb 11, 2015

cc @yungsters (for here and #3041) - I know it's been a while but you may remember some of your early thoughts around controlled components.

I'm not sold that returning to defaultValue is the right call. But in general this whole thing is a bit of a grey area. We should be consistent, regardless of which route we take.

@syranide
Copy link
Contributor

@zpao Returning to defaultValue is the only that makes sense to me, it would be the equivalent of treating them as separate modes/components, i.e. it resets when you switch between them. It was you who convinced me first :P

To me it becomes quite obvious when you think of React as being inherently kind of stateless, for a certain set of props you should always get the same output. So rendering <input defaultValue="test" /> sometimes rendering to "test" or ""/last value (without user interaction) depending on whether the input was previously controlled makes no sense to me, there's no transition between controlled/uncontrolled that really makes practical sense. They're different.

1. <input defaultValue="foo" />
   => "foo"
2. <input value="bar" />
   => "bar"
3. <input defaultValue="foobar" />
   => "foobar"
4. <input defaultValue="barfoo" />
   => "foobar"

Nothing else really makes sense to me in the context of React.

@zpao
Copy link
Member

zpao commented Feb 11, 2015

But if the user typed something, we leave that value in the DOM on subsequent renders. You could think of React as stateless but the DOM isn't and we respect that for uncontrolled components.

(same render fn I assume)
1. <input defaultValue="foo" />
   => "foo"
2. user types "bar"
   => "bar"
3. <input defaultValue="foobar" />
   => "bar"

If we treat changing the value prop like we treated a user manually changing the value, then we wouldn't return the defaultValue. I'm not saying that's the right thing, just that we have precedent.

@syranide
Copy link
Contributor

@zpao Yeah ofc. I improved my post a bit after I posted it. To me it comes down to the fact that transitioning from controlled to uncontrolled doesn't really make sense to me, you pick one and stick to it. You're not supposed to use controlled as a read-only state for your uncontrolled input. So if there's a controlled to uncontrolled transition, it's really a transition from <ControlledInput> to <UncontrolledInput> so it should reset... i.e. you forgot to give it a proper key.

EDIT: Or is there a use-case for transitioning from controlled to uncontrolled? Then it could make sense to keep the value, but I can't think of any...

@syranide
Copy link
Contributor

Also; it's easy to reproduce the behavior of the value sticking by simply updating defaultValue appropriately. But if it always keeps the last value then the only way to get it back is to manually detect the transition and update the value of the DOM node.

@syranide
Copy link
Contributor

@zpao Hmm, actually, let's rephrase it like this:

1. setState({value: 'foo'});
2. <input value={this.state.value} />
   => "foo"
3. (something happens)
4. setState({value: 'bar'});
5. <input value={this.state.value} />
6. <input defaultValue={this.state.value} />
   => "bar"

That was easy, now let's consider this instead:

1. setState({value: 'foo'});
2. <input value={this.state.value} />
   => "foo"
3. (something happens)
4. setState({value: 'bar'});
5. (noop or batched updates)
6. <input defaultValue={this.state.value} />
   => ???

I'm quite convinced now that the correct answer at step 6 is "bar" (i.e. it should reset to defaultValue). If it was "foo" you would be looking at an old irrelevant value, whether React re-rendered in-between (consider rAF-batching or whatever) should be irrelevant to the visual state shown to the user.

@martinstein
Copy link

@zpao I agree that the approach should be consistent.

The most consistent approach for value would be the one mentioned by @rymohr which is basically just one simple rule:

When a value is present, it is a controlled component. (... even if that value happens to be null).

Imagine we have a controlled component with a non-null value that gets changed to null. Now there are 2 interpretations:

  1. The component now becomes uncontrolled. That's the current behavior.
  2. @rymohr 's suggestion: The component stays controlled and the null value means we clear it.
    • This has no ambiguities.
    • enables intuitive clearing of a controlled component : simply set the value to null. I feel that this is almost a must-have behavior, because otherwise how would you clear the component?

What is the reasoning behind the current definition/behavior? If there aren't any good reasons besides "that's just the way it is defined", then it would probably be better to change the definition.

@syranide
Copy link
Contributor

@martinstein You still have transitions between controlled and uncontrolled inputs regardless of whether value={null} is considered controlled or not.

EDIT: I think turning value={null} into a controlled component is rather counter-intuitive, the solution should be to make uncontrolled and controlled inputs separate components, not using magic properties/values. Also, note that you can easily create your own wrapper to enable the behavior you prefer.

@martinstein
Copy link

@syranide You are right, there are still transitions between those two types. But there would be fewer ambiguities/inconsistencies.

Can you explain why you find value={null} equals controlled so counter-intuitive? I feel that if that behavior was intuitive, then we wouldn't need an extra page explaining this phenomenon:
http://facebook.github.io/react/tips/controlled-input-null-value.html

And the 'rule-set' would just be simpler.

Currently the rule is:

When a value is given, it is a controlled component. Except for a value of `null`, then it's uncontrolled.

The simpler rule would be:

When a value is given, it is a controlled component. Period.

Generally, the simpler approach is the better and more intuitive one, right?

@syranide
Copy link
Contributor

@martinstein It's not so much that I find <input value={null} /> to be controlled counter-intuitive, but that it breaks basic expectations of what a null value means and that is counter-intuitive. null means default/no value if you ask me, you would expect foo(null) === foo() to hold, just like I would argue that foo({enabled: null}) === foo({}) should hold and by extension <input value={null} /> === <input />. null is also commonly used to optimize hidden classes where it's generally expected to have the same behavior as if not having been specified unless for merging objects.

@martinstein
Copy link

I don't think it breaks basic expectations. Most people would expect that <input value={null}> means exactly that: "We have an input with an empty value". If that were supposed to mean: "We have an uncontrolled input", then why specify the value at all? And (sorry for the repetition) if that were intuitive, then that extra page in the docs would not be necessary.

From a language perspective, JavaScript specifically treats null different than not being defined at all:

var something = {test: null};
something.hasOwnProperty('test') // is true

That, for me is the expectation of how null should be evaluated. So foo({enabled: null}) === foo({}) should most definitely not hold!

I agree that it's sometimes used as a short-cut to optimize code but in this case it makes the code more complicated (my pull-request would have been simpler without the special treatment of null).

@rymohr
Copy link

rymohr commented Feb 12, 2015

I think we've collectively beaten this one to death at this point. Any new eyes from the core team that can take a look at it and give a fresh take?

@yungsters
Copy link
Contributor

I don't think that setting value to null or undefined should reset the DOM to defaultValue. If this behavior is desired, the composing component should continue setting value. Otherwise, setting value to an uncontrolled state should do just that — leave the DOM as-is. Letting component authors rely on conditionally setting value in order to reset defaultValue is pretty confusing, and I would not want anyone relying on this behavior even if we were consistent about it.

As for whether or not null should control the DOM, if we allow null to mean that a component's value is controlled, what is that value? For <input> and <textarea>, you may argue it should be an empty string. But what about a <select> with <option value="" /> and <option value="null" />? This ambiguity make me think that null should not control a prop value. Besides, it is very reasonable for component authors who want a controlled value of the empty string to set value="".

We should fix <input type="text" />.

@syranide
Copy link
Contributor

@yungsters But if we take my example above (copied below), I think the only thing that make sense is to "get the latest value and set it before becoming uncontrolled" which is equivalent to a reset.

The example from above, consider this scenario:

1. setState({value: 'foo'});
2. <input value={this.state.value} />
   => "foo"
3. (something happens)
4. setState({value: 'bar'});
5. <input value={this.state.value} />
6. <input defaultValue={this.state.value} />
   => "bar"

That's obvious, now let's consider this instead where the new state is not rendered due to batching (or w/e):

1. setState({value: 'foo'});
2. <input value={this.state.value} />
   => "foo"
3. (something happens)
4. setState({value: 'bar'});
5. (noop or batched updates)
6. <input defaultValue={this.state.value} />
   => ???

I would argue quite strongly that the value should still be "bar", whether or not the component actually performed intermediate re-renders should be irrelevant, especially as it's affected by the batching strategy. If we reset when becoming uncontrolled then it's always consistent.

That said however, I don't think going from controlled to uncontrolled is something you should do (the best solution would be if it wasn't at all possible if you ask me), we should simply make the best of it. But if you do, this is the only one that makes sense to me in the context of how React is meant to work, this is deterministic in a way that "keeping the last value" simply isn't.

@rymohr
Copy link

rymohr commented Oct 6, 2015

@syranide @jimfb 👍 sounds like a solid plan.

@syranide
Copy link
Contributor

syranide commented Oct 7, 2015

Valid use cases are exceedingly rare, and I'm thinking it's almost always a mistake.

@jimfb Expanding on that, I would go as far as saying it's either a mistake or bad practice. First; if controlled/uncontrolled where separate components (as they really should be IMHO), it would be impossible to switch like this. Second; it's trivial to implement uncontrolled behavior using a controlled input, so if what you really want is some non-standard behavior you're best off implementing your own component on-top of controlled inputs that implements whatever behavior you want, not switching between uncontrolled/controlled.

My point being; yes, there may be valid use-cases, but anyone being serious about their code can and should always avoid the warning as above. So I personally don't see any negative sides to having this warning.

@seeden
Copy link

seeden commented Oct 27, 2015

value || ''

is not a good solution because sometimes you have number 0 which is controlled too and you will lose this value

0 || '' => ''

here is acceptable fix:

function fixControlledValue(value) {
  if (typeof value === 'undefined' || value === null) {
    return '';
  }
  return value;
}

@iboxgithub
Copy link

Hi everyone,
this issue is still open so I guess nothing had been settled?
Even though IMO input and textarea should work the same (but I dont have a deep knowledge of it so I dont get in the conversation) I implemented what you guys wrote (btw good link
redux-form/redux-form#394)

Nevertheless, is this the final call? Or are we waiting for a confirmation this is the target implementation? (just to know if we will have to change our code anytime soon)

Cheers

@syranide
Copy link
Contributor

@iboxgithub You should not rely on this behavior and I believe 0.15 will issue a warning if you do.

@sebmarkbage
Copy link
Collaborator

You mean 15.0

On Feb 23, 2016, at 2:24 PM, Andreas Svensson notifications@github.com wrote:

@iboxgithub You should not rely on this behavior and I believe 0.15 will issue a warning if you do.


Reply to this email directly or view it on GitHub.

@iboxgithub
Copy link

Thanks for your feedback --> so what is THE best practice :)

@syranide
Copy link
Contributor

@iboxgithub An input should be either uncontrolled (value always undef/null) or controlled (value is a string, so it should be an empty string rather than null) for its entire lifetime.

@aweary
Copy link
Contributor

aweary commented Sep 19, 2017

I don't think there's anything actionable here, so I'm going to close this out. We still warn when trying to reset a controlled input using null, and still recommend @syranide's advice:

An input should be either uncontrolled (value always undef/null) or controlled (value is a string, so it should be an empty string rather than null) for its entire lifetime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

15 participants