Eliminate unnecessary numeric equality checks by nhunzaker · Pull Request #11751 · facebook/react

nhunzaker · 2017-12-02T18:09:56Z

This commit changes the way numeric equality for number inputs works such that it compares loose equality for numbers. This seems sufficient, and eliminates quite a bit of branching around numeric equality.

This additionally addresses Flow type issues on #11741, and it is based off that branch

Test plan:

Open http://react-concise-numbers.surge.sh
Run through the Text and Number test fixtures

Tested in:

This commit changes the way numeric equality for number inputs works such that it compares against `input.valueAsNumber`. This eliminates quite a bit of branching around numeric equality.

gaearon · 2017-12-02T18:19:04Z

Can you explain why the original code was written this way, and why it's no longer necessary despite comments saying something about IE9?

nhunzaker · 2017-12-02T18:39:19Z

packages/react-dom/src/client/ReactDOMFiberInput.js

-      ) {
+    if (props.type === 'number') {
+      // eslint-disable-next-line
+      if (node.valueAsNumber !== value && node.value != value) {


Actually I wonder if I even need to check valueAsNumber. Everything works as expected in IE9, which does not support this API.

nhunzaker · 2017-12-02T18:50:24Z

packages/react-dom/src/client/ReactDOMFiberInput.js

-      ) {
+    if (props.type === 'number') {
+      // eslint-disable-next-line
+      if (node.value != value) {


This seems to be too good to be true, but this checks out in every browser. Unit tests also confirm it. Have I missed something?

Worth checking git blame to see why this was introduced.

nhunzaker · 2017-12-02T19:08:26Z

packages/react-dom/src/client/ReactDOMFiberInput.js

-      ) {
+    if (props.type === 'number') {
+      // eslint-disable-next-line
+      if (node.value != value || (value === 0 && node.value === '')) {


Looks like we didn't cover "" to 0 in our unit tests. Added.

nhunzaker · 2017-12-02T19:22:40Z

Can you explain why the original code was written this way [...]

Identifying all of the edge cases was a long road, and we didn't have all of the test fixtures and unit tests to cover edge cases in type coercion. It was the result of adding additional branches to cover different cases.

[...] and why it's no longer necessary despite comments saying something about IE9?

IE9 does not support input.valueAsNumber, so we ran parseFloat on the value. However it looks like this is not necessary. The comment about IE9 justifies why we have to do that.

I think the gist of this is just that we do not need to parse the value as a number. We can make the following reductions (or at least this is my logic so we can try to poke holes in it):

if (props.type === 'number') {
     // Simulate `input.valueAsNumber`. IE9 does not support it
     var valueAsNumber = parseFloat(node.value) || 0;
     if (
       // eslint-disable-next-line
       value != valueAsNumber ||
       // eslint-disable-next-line
       (value == valueAsNumber && node.value != value)
     ) {

value == valueAsNumber after doing a check for value != valueAsNumber is extraneous. We can collapse the check into:

if (props.type === 'number') {
     // Simulate `input.valueAsNumber`. IE9 does not support it
     var valueAsNumber = parseFloat(node.value) || 0;
     if (
       // eslint-disable-next-line
       value != valueAsNumber && node.value != value
     ) {

To the best of my ability to test, == and != have the same effect on a string as parseFloat. That means we can remove it:

if (props.type === 'number') {
     // eslint-disable-next-line
     if (value != node.value && node.value != value) {

This is the same check! So finally we get to:

if (props.type === 'number') {
     // eslint-disable-next-line
     if (value != node.value) {

The edge case here is that '' loose equals 0. That is controlled for in a branch earlier in the function, but we can collapse them to the same line:

if (props.type === 'number') {
     // eslint-disable-next-line
     if (if (node.value != value || (value === 0 && node.value === '')) {

aweary · 2017-12-03T20:36:29Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

    }
  });

+  it('performs as state change from "" to 0', () => {


"performs a state change"

Got it in 2ba3fe8

aweary · 2017-12-03T20:37:35Z

packages/react-dom/src/client/ReactDOMFiberInput.js

-        (value == valueAsNumber && node.value != value)
-      ) {
+    if (props.type === 'number') {
+      // eslint-disable-next-line


Can we update this so it's only disabling the coercion rule?

Great idea. I also moved the loose check after the value === 0 guard to avoid some extra DOM access. 3cdde77

aweary · 2017-12-03T20:39:34Z

packages/react-dom/src/client/ReactDOMFiberInput.js

-      ) {
+    if (props.type === 'number') {
+      // eslint-disable-next-line
+      if (node.value != value || (value === 0 && node.value === '')) {


Before this change the 0/ "" coercion would occur for any type of input. Now it only applies to number inputs. Any chance that breaks something weird?

Not from my ability to test. The strict equality check covers it.

aweary

It does look like parseFloat and == are logically equivalent in this situation. I can't think of any situation where it fails, so assuming all the fixtures pass in the supported browsers this LGTM

nhunzaker · 2017-12-04T13:16:23Z

I'll check this into #11741 in a bit and that one should be good to go.

…1741) * Ensure value and defaultValue do not assign functions and symbols * Eliminate assignProperty method from ReactDOMInput * Restore original placement of defaultValue reservedProp * Reduce branching. Make assignment more consistent * Control for warnings in symbol/function tests * Add boolean to readOnly assignments * Tweak the tests * Invalid value attributes should convert to an empty string * Revert ChangeEventPlugin update. See #11746 * Format * Replace shouldSetAttribute call with value specific type check DOMProperty.shouldSetAttribute runs a few other checks that aren't appropriate for determining if a value or defaultValue should be assigned on an input. This commit replaces that call with an input specific check. * Remove unused import * Eliminate unnecessary numeric equality checks (#11751) * Eliminate unnecessary numeric equality checks This commit changes the way numeric equality for number inputs works such that it compares against `input.valueAsNumber`. This eliminates quite a bit of branching around numeric equality. * There is no need to compare valueAsNumber * Add test cases for empty string to 0. * Avoid implicit boolean JSX props * Split up numeric equality test to isolate eslint disable command * Fix typo in ReactDOMInput test * Add todos * Update the attribute table

Eliminate unnecessary numeric equality checks

979da6d

This commit changes the way numeric equality for number inputs works such that it compares against `input.valueAsNumber`. This eliminates quite a bit of branching around numeric equality.

nhunzaker requested review from aweary, gaearon and jquense December 2, 2017 18:09

nhunzaker mentioned this pull request Dec 2, 2017

Ensure value and defaultValue do not assign functions and symbols #11741

Merged

nhunzaker commented Dec 2, 2017

View reviewed changes

There is no need to compare valueAsNumber

dad111d

nhunzaker commented Dec 2, 2017

View reviewed changes

Add test cases for empty string to 0.

0a34070

nhunzaker commented Dec 2, 2017

View reviewed changes

Avoid implicit boolean JSX props

9ab1e7d

facebook-github-bot added the CLA Signed label Dec 2, 2017

aweary reviewed Dec 3, 2017

View reviewed changes

aweary approved these changes Dec 3, 2017

View reviewed changes

nhunzaker added 2 commits December 4, 2017 08:11

Split up numeric equality test to isolate eslint disable command

3cdde77

Fix typo in ReactDOMInput test

2ba3fe8

gaearon merged commit a65e25f into fix-value-assignment Dec 4, 2017

gaearon deleted the nh-more-concise-input-numeric-equality branch December 4, 2017 13:50

Conversation

nhunzaker commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Dec 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhunzaker commented Dec 2, 2017

Uh oh!

aweary Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweary left a comment

Choose a reason for hiding this comment

Uh oh!

nhunzaker commented Dec 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nhunzaker commented Dec 2, 2017 •

edited

Loading

aweary Dec 3, 2017 •

edited

Loading