fix textarea value of number 0

[chenglou]

previously, setting value to number 0 is treated as if value wasn't set at all (thus the textarea is cleared from 0 to '' upon onChange.

[yungsters]

Heh, look at the condition surrounding this update. I think you want to do what ReactDOMInput does:

'' + this.props.value || ''

[chenglou]

ew lol, sorry about that

[yungsters]

Actually, I think that || '' is redundant. Maybe. Anyway, thanks for fixing this. I'll merge it when I get to a desktop.

[chenglou]

true, also changed the one in input, although it more or less belong here?

[zpao]

That code works, but we should have some tests to prove it 😉

[chenglou]

here are the tests. I combined two checks into one (checking nothing changes, and checking for the specific bug in this issue). Not sure if it's a good idea.

[sophiebits]

Obviously the old code here differs from textarea already, but perhaps this was intentional so that false becomes ''; generally in React you can write flag && <Component /> where nothing is inserted if flag is false so here perhaps we want to support <input value={flag && val} /> here too.

[zpao]

Ok, 1 week is too long to let this sit. I pulled it in last week but it failed a test internally which was depending on value={false} to work, and then I got distracted by other things. Sorry about that!

Those tests that fail also have tests that are checking that value={true} result in value="true", which made me think that it's a bit awkward that 1 bool works while the other doesn't. Worse, other props will still output "false", eg title={false} results in title="false" in the DOM. That said, I think this is fine and no worse than we are right now, this diff just made me realize the inconsistency.

So I need to do a little bit more fixing of some custom components internally before I can pull this in, but soon!

[chenglou]

I can undo the changes if we decide on a consistent way to display false.

[sophiebits]

From IRC last night it sounds like we want to turn booleans and all other non-string/number things into ''. Not sure off the top of my head how this compares to what is currently being done in DOMPropertyOperations in general or for children in ReactNativeComponent…

[zpao]

I think this is still the right thing to take right now, and then we should revisit the rest of the issues. You aren't making anything worse with these changes, just strictly better. '' + true || '' was still === 'true'

[yungsters]

Everywhere else in React, true and false evaluate to the strings 'true' and 'false', respectively.

// <span ... title="true"></span>
React.renderComponent(React.DOM.span({title: true}), container);
// <span ... title="false"></span>
React.renderComponent(React.DOM.span({title: false}), container);

Why don't we want to keep this behavior?

[zpao]

Well, this diff doesn't change anything except making value={0} work. value={false} is unchanged (in other words, still inserts empty string), as is value={true} (inserts "true").

I think we should be consistent (that's always my theme). Do we always allow primitives? Do we only special case undefined and null, but allow all other primitives? Do we restrict to number and string? Or maybe we just always .toString() whatever you give us? The current mix here is bad and we should change it, totally agree. But let's do it separately and fix some other consistencies while we're at it...

Let's take a look at some crazy shit...

// It's a String!!!!
<div title={new String('title')}>{true}{false}{null}{undefined}{0}{{lol: 'wat'}}</div>;
// Error: Invariant Violation: escapeTextForBrowser(...): Attempted to escape an object.

// Ok, so {lol: 'wat'} makes sense when you realize that's how keys work... But then we only render string or number literals...
<div>{true}{false}{null}{undefined}{0}</div>;
// <div><span>0</span></div>

// Wait for it...
<div>{true}{false}{null}{undefined}{0}{new String('hi tim')}</div>;
// <div><span>0</span><span>h</span><span>i</span><span></span><span>t</span><span>i</span><span>m</span></div>

[petehunt]

This hurts my brain because nullity in JS confuses the hell out of me. Can we instead make a new module that does something like this?

function convertToDOMString(v) {
  if (v === false || v === undefined || v === null) {
    // These special cases should never have their toString() representation rendered into the DOM
    return '';
  }
  // Everything else? toString().
  return '' + v;
}

I understand ===, I don't understand ==.

[zpao]

Hey @chenglou, can you rebase? (feel free to squash too if you want). Let's ignore @petehunt's comment since @yungsters accepted and Pete's getting on a plane soon anyway. We'll come back to what he said when we figure out the real consistent behavior here.

[benjamn]

Curious why the node.value = 'giraffe' is ignored here.

[sophiebits]

It's a controlled component with no onChange handler… (note I'm changing the actual value, not the value prop).

[benjamn]

So ReactTestUtils.Simulate.input(node) should cause node.value to be reset? Do you happen to know where that happens?

[zpao]

https://github.com/facebook/react/blob/master/src/dom/components/ReactDOMInput.js#L81

[sophiebits]

Yeah, the input event triggers the onChange handler in ReactDOMInput which sets value on state to the current node value giraffe, causing a rerender after which componentDidUpdate sets node.value to match props.value. Something not working for you?

[benjamn]

I'm trying to upgrade the jsdom module that we use for our internal test environment. It's a big jump (v0.2.15 to v0.8.6), so no surprise there are problems, but one of them happens to be that node.value remains "giraffe" after this input event. I'll investigate some more. Thanks for confirming this is the expected behavior.