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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 0 commits into from
Closed

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

wants to merge 0 commits into from

Conversation

rickbeerendonk
Copy link
Contributor

Fixes #5773

@facebook-github-bot
Copy link

@rickbeerendonk updated the pull request.

@rickbeerendonk rickbeerendonk force-pushed the 5773-Warn-if-style-value-is-NaN branch 2 times, most recently from 45ebb10 to 7c5390f Compare January 9, 2016 21:30
@facebook-github-bot
Copy link

@rickbeerendonk updated the pull request.

@syranide
Copy link
Contributor

Isn't isNaN already available on all browsers? Also, value !== value is not a sufficient test.

@jimfb
Copy link
Contributor

jimfb commented Jan 10, 2016

@syranide isNaN is available everywhere, but it has some nasty edge cases. For instance, the String "NaN" will return true. Number.isNaN is much better, but is not yet supported on all browsers. value !== value is one of the recommended polyfills - why is it not sufficient?

@syranide
Copy link
Contributor

@jimfb Oh wow that's interesting, didn't know about that. Yeah my bad, it's correct. I was thinking of 0 !== -0 from the Object.is polyfill, but my brain fooled me, it was the other way around. 👍

@syranide
Copy link
Contributor

However, we don't want to polyfill Number.isNaN in the global namespace. We should require users to have the polyfill or make the polyfill internal.

@facebook-github-bot
Copy link

@rickbeerendonk updated the pull request.

@zpao
Copy link
Member

zpao commented Jan 11, 2016

All the same things I said in #5811. We should pick one

@zpao zpao closed this Jan 11, 2016
@zpao zpao reopened this Jan 11, 2016
@facebook-github-bot
Copy link

@rickbeerendonk updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rickbeerendonk @facebook-github-bot @syranide @jimfb @zpao