Warn when a style object has NaN for a value #5811

jontewks · 2016-01-08T23:45:36Z

Warn when a style object has NaN for a value instead of showing mutated style warning.

Fix for issue #5773

jimfb · 2016-01-09T00:36:35Z

src/renderers/dom/shared/ReactDOMComponent.js

+  for (var key in style1) {
+    var value = style1[key];
+
+    if (isNaN(value) && ('' + value) === 'NaN') {


I don't think the isNaN(value) && ('' + value) === 'NaN' is correct. For instance, the string "NaN" would pass that check, I believe.

You probably want to use Number.isNaN() or one of the polyfills listed in the docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isNaN

Thanks for that. That's basically what I was trying to emulate but somehow missed the more specific Number.isNaN in my search. Updated with this change.

facebook-github-bot · 2016-01-09T00:43:14Z

@jontewks updated the pull request.

facebook-github-bot · 2016-01-09T00:57:06Z

@jontewks updated the pull request.

jimfb · 2016-01-09T04:40:21Z

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js

-        'mutated. Mutating `style` is deprecated. Consider cloning it ' +
-        'beforehand. Check the `render` using <span>. Previous style: ' +
-        '{fontSize: NaN}. Mutated style: {fontSize: NaN}.'
+        'Warning: You passed NaN as a value for fontSize. Are you sure this is intentional?'


It might not be immediately obvious to a user that we're talking about css styles here, it might be good to mention that it's a css style. Also, I'm not sure any of our other warnings end in a question mark. Maybe something like:

NaN is an invalid value for the fontSize css style property

facebook-github-bot · 2016-01-09T05:16:20Z

@jontewks updated the pull request.

facebook-github-bot · 2016-01-09T06:06:37Z

@jontewks updated the pull request.

jimfb · 2016-01-09T07:31:07Z

src/renderers/dom/shared/ReactDOMComponent.js

+  // so they don't give a false positive
+  for (var key1 in style1) {
+    if (numberIsNaN(style1[key1])) {
+      delete style1[key1];


I still feel like we probably shouldn't be modifying the argument.

This is a dev-only code path, so maybe we copy the object and mutate the copy. Or maybe we fork shallowEqual to have the logic we want with regards to NaN in this case. Or maybe there is another option/thought.

cc @sebmarkbage @spicyj

@jimfb The shallow-equal check should really use Object.is, then this problem should go away naturally.

@jimfb Opened a pull request to fix shallowEqual().

Thanks @syranide , I agree, that's better.

@jontewks Let's get rid of the changes to this file. We can fix shallowEqual separately, since that's in a different repository.

facebook-github-bot · 2016-01-10T00:49:23Z

@jontewks updated the pull request.

sophiebits · 2016-01-10T05:46:11Z

@zpao just updated shallowEqual internally to use an Object.is polyfill which we should sync out soon.

Also why not use just x !== x to check if x is NaN?

jontewks · 2016-01-10T06:14:12Z

Thanks, I'll look for that to update. The x !== x is the second polyfill on MDN of Number.isNaN, which I thought was pretty clever, but it does seem like updating shallowEqual was the best option so it could propagate elsewhere as well and then defer to that no?

sophiebits · 2016-01-10T06:19:30Z

Yes. @zpao wrote a change for shallowEqual last week, just needs to be applied to fbjs.

zpao · 2016-01-11T19:36:48Z

src/renderers/dom/shared/CSSPropertyOperations.js

@@ -106,6 +120,10 @@ if (__DEV__) {
    } else if (badStyleValueWithSemicolonPattern.test(value)) {
      warnStyleValueWithSemicolon(name, value);
    }
+
+    if (numberIsNaN(value)) {


Let's just inline the check here.

The advantage to not inlining is readability, since otherwise if(value !== value) has an incredibly obscure behavior. Easy to have a 3am WTF when your eyes are tired and brain is in low-power mode. I don't feel strongly, so if you want it inlined, that's fine with me.

Don't inline the short one then. if (typeof value === 'number' && isNaN(value))

facebook-github-bot · 2016-01-12T05:55:35Z

@jontewks updated the pull request.

jontewks · 2016-01-15T21:42:15Z

Hey all, I just noticed the change to fbjs that will fix the test for this PR has arrived, but there wasn't a new release version made for it yet, so npm install doesn't grab the newest code. This PR will be good to go once fbjs version is bumped and I find out the final decision on moving that unit test out or not.

jimfb · 2016-01-15T21:51:17Z

cc @zpao for release planning/scheduling.

jontewks · 2016-01-16T00:18:08Z

Thanks @zpao, all updated.

facebook-github-bot · 2016-01-16T00:18:44Z

@jontewks updated the pull request.

jontewks · 2016-01-16T00:31:32Z

Just squashed everything into one commit as well. Thanks again!

jontewks · 2016-01-16T02:46:18Z

@zpao Updated per your recent comment that was wiped away in the squash and force update regarding the tests in ReactDOMComponent-test.js

facebook-github-bot · 2016-01-16T02:50:42Z

@jontewks updated the pull request.

facebook-github-bot · 2016-01-16T19:13:50Z

@jontewks updated the pull request.

zpao · 2016-01-19T01:06:18Z

Looks great, thanks @jontewks!

Warn when a style object has NaN for a value

facebook-github-bot added GH Review: review-needed CLA Signed labels Jan 8, 2016

jontewks mentioned this pull request Jan 8, 2016

Warn if style value is NaN #5773

Closed

jimfb reviewed Jan 9, 2016
View reviewed changes

zpao reviewed Jan 11, 2016
View reviewed changes

zpao mentioned this pull request Jan 11, 2016

Warn on initial render when style contains a NaN value. #5816

Closed

jontewks mentioned this pull request Jan 12, 2016

Warn on initial render when style contains a NaN value. #5822

Closed

jontewks force-pushed the warn-nan-style branch from 7481db4 to 8ca7e93 Compare January 16, 2016 00:31

jontewks force-pushed the warn-nan-style branch from 8ca7e93 to 3e33cea Compare January 16, 2016 02:43

Add warning for NaN in style object

7a6000c

jontewks force-pushed the warn-nan-style branch from 3e33cea to 7a6000c Compare January 16, 2016 19:11

zpao added a commit that referenced this pull request Jan 19, 2016

Merge pull request #5811 from jontewks/warn-nan-style

31d3bfa

Warn when a style object has NaN for a value

zpao merged commit 31d3bfa into facebook:master Jan 19, 2016

zpao added GH Review: accepted and removed GH Review: review-needed labels Jan 19, 2016

renovate bot mentioned this pull request Feb 2, 2018

Update dependency react to v0.14.9 - autoclosed signavio/backbone-rel#29

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when a style object has NaN for a value #5811

Warn when a style object has NaN for a value #5811

jontewks commented Jan 8, 2016

jimfb Jan 9, 2016

jontewks Jan 9, 2016

facebook-github-bot commented Jan 9, 2016

facebook-github-bot commented Jan 9, 2016

jimfb Jan 9, 2016

facebook-github-bot commented Jan 9, 2016

facebook-github-bot commented Jan 9, 2016

jimfb Jan 9, 2016

syranide Jan 9, 2016

rickbeerendonk Jan 9, 2016

jimfb Jan 9, 2016

facebook-github-bot commented Jan 10, 2016

sophiebits commented Jan 10, 2016

jontewks commented Jan 10, 2016

sophiebits commented Jan 10, 2016

zpao Jan 11, 2016

jimfb Jan 11, 2016

zpao Jan 11, 2016

facebook-github-bot commented Jan 12, 2016

jontewks commented Jan 15, 2016

jimfb commented Jan 15, 2016

jontewks commented Jan 16, 2016

facebook-github-bot commented Jan 16, 2016

jontewks commented Jan 16, 2016

jontewks commented Jan 16, 2016

facebook-github-bot commented Jan 16, 2016

facebook-github-bot commented Jan 16, 2016

zpao commented Jan 19, 2016

Warn when a style object has NaN for a value #5811

Warn when a style object has NaN for a value #5811

Conversation

jontewks commented Jan 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 9, 2016

facebook-github-bot commented Jan 9, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Jan 9, 2016

facebook-github-bot commented Jan 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 10, 2016

sophiebits commented Jan 10, 2016

jontewks commented Jan 10, 2016

sophiebits commented Jan 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 12, 2016

jontewks commented Jan 15, 2016

jimfb commented Jan 15, 2016

jontewks commented Jan 16, 2016

facebook-github-bot commented Jan 16, 2016

jontewks commented Jan 16, 2016

jontewks commented Jan 16, 2016

facebook-github-bot commented Jan 16, 2016

facebook-github-bot commented Jan 16, 2016

zpao commented Jan 19, 2016