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

NaN assertion is not strict enough #498

Closed
olivr70 opened this issue Jul 23, 2015 · 3 comments
Closed

NaN assertion is not strict enough #498

olivr70 opened this issue Jul 23, 2015 · 3 comments

Comments

@olivr70
Copy link

olivr70 commented Jul 23, 2015

NaN was recently hopefully recently added.

But because it relies on the global isNaN() function, it has the same flaws : because isNan() converts its argument before testing, it returns true in many unexpected cases, and false in others. It is not very predictible (i.e. isNan("bla") returns true, but isNaN("") returns false).

ES6 introduces Number.isNaN() which behaves as intuitively expected : if the value is a NaN, it returns true, otherwise (strings, dates...), it returns false. A very short polyfill for ES5 is proposed in the MDN documentation.

A strict isNaN is really needed. I can see 2 options :

  1. rewriting the current NaN property using the MDN polyfill, but this would be a breaking change
  2. implementing a strict NaN testing, which could be either
  3. as a simple Property : .striclyNaN
  4. using a flag like .stricly.NaN (expect(x).to.be.stricly.NaN;

I really think current isNaN() implementation is nearly useless, so I prefer the first option.

If the preferred way is option 2, then option 2.1 feels more like in the spirit of chai, which has different predicates for loose and strict versions of the same predicate (above/least, below/most, equal/eql).

I'm OK to do the job when this discussion will be closed.

@keithamus
Copy link
Member

Lets make the breaking change - as you say, its useless if it throws up false positives.

There's a 4.x.x branch which you can make a PR into. When we release 4.0.0 we can release it with the breaking change 😄

@berkerpeksag
Copy link

This is fixed in cbfb6fd (in the 4.x.x branch, not master) and can be closed now.

@keithamus
Copy link
Member

Thanks again @berkerpeksag 😄

lucasfcosta pushed a commit to lucasfcosta/chai that referenced this issue Mar 14, 2016
lucasfcosta pushed a commit to lucasfcosta/chai that referenced this issue Mar 14, 2016
meeber pushed a commit to meeber/chai that referenced this issue Sep 5, 2016
meeber pushed a commit to meeber/chai that referenced this issue Sep 8, 2016
lucasfcosta pushed a commit to lucasfcosta/chai that referenced this issue Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants