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

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

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
7 participants
@jontewks
Copy link
Contributor

commented Jan 8, 2016

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

Fix for issue #5773

for (var key in style1) {
var value = style1[key];

if (isNaN(value) && ('' + value) === 'NaN') {

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

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

This comment has been minimized.

Copy link
@jontewks

jontewks Jan 9, 2016

Author Contributor

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.

var value = style1[key];

if (isNaN(value) && ('' + value) === 'NaN') {
return;

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

This will bail out if any of the styles are NaN, but I don't think that's what we want. For instance, suppose one of the styles is NaN but the other style was mutated. In such a case, we want to display the mutation warning for the style that was mutated, and just skip the NaN.

This comment has been minimized.

Copy link
@jontewks

jontewks Jan 9, 2016

Author Contributor

I originally thought the NaN warning would suffice, but you are right and it would be a good idea to show both. I've updated the PR to remove the NaN values before the equality check for the mutation warning.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 9, 2016

@jontewks updated the pull request.

1 similar comment
@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 9, 2016

@jontewks updated the pull request.

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

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

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

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

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

I feel like we probably shouldn't be modifying the user's input object. cc @sebmarkbage @spicyj

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

if (Number.isNaN(value)) {

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

Number is not being imported at the top of this file. I think you'll need to create a Number polyfill module (ie. just copy-paste the one from the MDN docs) and import it in order to support legacy browsers.

This comment has been minimized.

Copy link
@jontewks

jontewks Jan 9, 2016

Author Contributor

Jim, I've added a module that exports that function rather than polyfilling onto the Number object. I found a file that appeared to be doing this for Object.assign, but I couldn't tell if that was being used anymore since there is also a node_module object-assign, but tried to follow this convention anyways. Let me know if this is the wrong place for this file, or if adding the isNaN function onto the Number object is preferable, and where that file should be placed. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 9, 2016

@jontewks updated the pull request.

1 similar comment
@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 9, 2016

@jontewks updated the pull request.

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

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

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

This comment has been minimized.

Copy link
@syranide

syranide Jan 9, 2016

Contributor

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

This comment has been minimized.

Copy link
@rickbeerendonk

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

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.

'use strict';

/*
* Polyfill for the Number.isNaN function

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 9, 2016

Contributor

TODO: @zpao what is the correct copyright header here?

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 10, 2016

@jontewks updated the pull request.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2016

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

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

if (numberIsNaN(value)) {

This comment has been minimized.

Copy link
@zpao

zpao Jan 11, 2016

Member

Let's just inline the check here.

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 11, 2016

Contributor

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.

This comment has been minimized.

Copy link
@zpao

zpao Jan 11, 2016

Member

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

@@ -94,6 +95,19 @@ if (__DEV__) {
);
};

var warnStyleValueIsNaN = function(name, value) {
if (warnedStyleValues.hasOwnProperty(value) && warnedStyleValues[value]) {

This comment has been minimized.

Copy link
@zpao

zpao Jan 11, 2016

Member

Please don't reuse warnedStyleValues - it's used for tracking semicolons in values. We can just have a global boolean warnedForNaNValue (we can only warn for NaN once).

It's unfortunate that we don't have more context in here - once and done for this warning isn't the most helpful thing (but better than nothing)

'mutated. Mutating `style` is deprecated. Consider cloning it ' +
'beforehand. Check the `render` using <span>. Previous style: ' +
'{fontSize: NaN}. Mutated style: {fontSize: NaN}.'
'Warning: `NaN` is an invalid value for the `fontSize` css style property'

This comment has been minimized.

Copy link
@zpao

zpao Jan 11, 2016

Member

We should be probably just remove this test and write a new one in CSSPropertyOperations-test (since we are now testing that unit not ReactDOMComponent - this one is just surfacing the warning that was emitted before)

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 11, 2016

Contributor

I actually want to move the warnings out into a separate module eventually (using devtools api), so if we move it to CSSPropertyOperations, I would just have to move it back here later. I think it's fine to leave it here, since then this is testing public api instead of internal API, which is better anyway.

This comment has been minimized.

Copy link
@zpao

zpao Jan 11, 2016

Member

It should be moved. We should be doing unit testing. We're already testing the other warnings from that module there.

This comment has been minimized.

Copy link
@jimfb

This comment has been minimized.

Copy link
@jontewks

jontewks Jan 12, 2016

Author Contributor

Please let me know the final verdict on if this should be moved or not. I believe that we will be all set once that is finalized and the fbjs shallowEqual is updated.

This comment has been minimized.

Copy link
@zpao

zpao Jan 16, 2016

Member

Let's do this (mostly what I said but updated for correctness):

  • Leave this test but we need to update the 1 on line 199 to a 2 (to account for the warning you're adding), and then update the argsForCall[0][0] to argsForCall[1][0]. We'll remove this test entirely when the fbjs update comes along, but that update doesn't block this PR.
  • Add a new test explicitly for the new warning in CSSPropertyOperations-test
  • Ignore what @jimfb said (it's relevant but not to you right now)

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 16, 2016

Contributor

👍

This comment has been minimized.

Copy link
@zpao

zpao Jan 16, 2016

Member

We actually should leave this message in tact and just update which call's arguments we look at - there will actually be 2 warnings fired, the first is your new warning and the 2nd is the one that was here. In this test we should look for the "modified" style object (even if it is a lie right now).

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 12, 2016

@jontewks updated the pull request.

@jontewks

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

cc @zpao for release planning/scheduling.

@jontewks

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2016

Thanks @zpao, all updated.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 16, 2016

@jontewks updated the pull request.

@jontewks jontewks force-pushed the jontewks:warn-nan-style branch from 7481db4 to 8ca7e93 Jan 16, 2016

@jontewks

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2016

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

@jontewks jontewks force-pushed the jontewks:warn-nan-style branch from 8ca7e93 to 3e33cea Jan 16, 2016

@jontewks

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2016

@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

This comment has been minimized.

Copy link

commented Jan 16, 2016

@jontewks updated the pull request.

@jontewks jontewks force-pushed the jontewks:warn-nan-style branch from 3e33cea to 7a6000c Jan 16, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 16, 2016

@jontewks updated the pull request.

@zpao

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

Looks great, thanks @jontewks!

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

Merge pull request #5811 from jontewks/warn-nan-style
Warn when a style object has NaN for a value

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.