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

Use lodash.isnan instead of Number.isNaN #584

Closed
wants to merge 1 commit into from

Conversation

ohbarye
Copy link

@ohbarye ohbarye commented Apr 16, 2018

Hello, maintainers.

I'm trying to solve #559 with this PR by adding lodash.isnan as a dependency since IE doesn't support Number.isNaN.

ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isNaN

But I'm not sure whether this is the best approach to make polyfill for this project, so if you have a better solution in your mind I'll comply it.

@prichodko
Copy link
Collaborator

Libraries shouldn't include any polyfills. However, I wouldn't consider this change as adding polyfill and rather using better module for this operation. I am 👍 .

@jaredpalmer
Copy link
Owner

This is kinda hard. I’d rather document a polyfill than bloat the lib

@prichodko
Copy link
Collaborator

prichodko commented Apr 18, 2018

When you inspect the code, it's not really a polyfill, is it? It just uses a different technique to determine NaN. On the other hand, I understand the point.

@vlad-zhukov
Copy link
Contributor

vlad-zhukov commented Apr 18, 2018

There is no need to bring another dependency because isNaN is a simple value !== value check. See #588 for an alternative to this PR.

@prichodko
Copy link
Collaborator

TIL

@ohbarye ohbarye deleted the isnan-polyfill-for-ie branch April 19, 2018 00:39
@ohbarye
Copy link
Author

ohbarye commented Apr 19, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants