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

isNaN and isFinite should accept number or string #4002

Closed
darrylring opened this issue Jul 23, 2015 · 20 comments
Closed

isNaN and isFinite should accept number or string #4002

darrylring opened this issue Jul 23, 2015 · 20 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@darrylring
Copy link

According to the ES5.1 spec (15.1.2.4, 15.1.2.5), isNaN(number) and isFinite(number) perform the ToNumber(number) abstract operation before checking if number coerces to NaN or ±Infinity.

To me, this implies that the argument to these functions can be any type. I think TypeScript should follow this as well. Why check if a variable is a number before checking if it's not a number (NaN)? Why manually coerce a variable into a number before calling a function that, according to the specification, does just that?

@RyanCavanaugh
Copy link
Member

perform the ToNumber(number) abstract operation

The same is true of many other functions -- Math.max, for example, but we don't allow Math.max(window, 'world').

What's some example code where you'd intentionally pass a non-number to those functions?

@darrylring
Copy link
Author

At the very least it should allow a string. "4" is finite. My use case is having a variable of type string | number representing a position -- similar to CSS's vertical-align, it could be "left", "right", "center", or 50, or -10.

if (position == 'right' || (isFinite(position) && position < 0)) {

If isFinite(position) == true, then position is numeric enough to compare to 0. It doesn't matter if position is -4 or "-4", it's finite, and it's less than 0. If it's "left", it's not finite.

Edit: I would make the same argument for Math.max, et al. Math.max("4", 3) gives you the number 4.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 23, 2015

That sort of unintended coercion I see as one of the major use cases for TypeScript, as there is just too much "flexibility" in ES. I personally would much prefer to throw an error and then for me to explicitly coerce it. It make my code more explicit. While technically it isn't needed at runtime, that is almost the whole point of TypeScript, isn't it?

@RyanCavanaugh
Copy link
Member

Implicit coercions are the bane of JavaScript. 99% of the 'wat' things people make fun of JS for are implicit coercions.

Thinking about Math.max and what its signature should be:

  • Math.max("4", 3) is 4, which is not === to any of its inputs
  • Math.max(" \r\n\t ", -1) is 0, which again is not either of its inputs
  • Math.max(10, [11]) is 11. So we should allow number[] as an input to max because it produced a possibly-meaningful result once?
  • Math.max(10, [11, 12]) is NaN. So... maybe not number[] ? Or we should assume you know it's only safe to pass single-element arrays to it?

Just because a function produces a result doesn't mean that result is a meaningful one. These are the kind of patterns we'd prefer to error on. isFinite("") === true and "" >= 0, but it's really not clear that anyone wants to treat "" as if it were actually a number.

There's a single-character fix: isFinite(+position). This makes it clear that you want to coerce position to a number.

@darrylring
Copy link
Author

I agree with both of you, but I would suggest it's a matter of degree. Let's ignore Math.max() because, as you say, the output can be !== to any of its inputs. I was not originally thinking about it, so my comment was just off-hand.

isFinite("hello") == false is neither unexpected nor unintended. The result makes sense and is meaningful. I understand the + coercion, but I do not agree that it is more clear about anything. Nothing plus a string is a number?

@darrylring darrylring changed the title isNaN and isFinite should accept any value isNaN and isFinite should accept number or string Jul 23, 2015
@darrylring
Copy link
Author

+"2" // => 2
1+"2" // => "12"

Tell me how this is more explicit or clear. JavaScript is the worst.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 23, 2015

Well, that is why I don't like the simplified coercions... I prefer to use the more explicit:

Number("2"); // => 2
1 + Number("2"); // => 3

Because of exactly that... No one is going to get confused (including the runtime) about what I am trying to do, including myself. More typing, sure... but far less surprising.

@darrylring
Copy link
Author

My thinking is that in the case of isFinite and isNaN, they tell you if something can be coerced into a number that is finite or not. If you're going to use it as a number, it's going to have to be coerced anyways:

if (isFinite(y)) { x += Number(y); }
if (isFinite(Number(y))) { x += Number(y); }

I guess I just don't really see what the second line buys you over the first, at least as far as y being a number or a string goes.

@RyanCavanaugh
Copy link
Member

I do not agree that it is more clear about anything. Nothing plus a string is a number?

Unary + is in nearly every programming language -- it might be unclear if you've never seen it before, but the same is true of every operator.

The real problem here is that calling isFinite is a meaningful and well-defined operation on numbers, and an incredibly tricky operation on non-numbers with many counter-intuitive results. Doing the tricky operation by default is dangerous. If you want to perform a coercion, do it explicitly with unary +.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Declined The issue was declined as something which matches the TypeScript vision labels Jul 23, 2015
@darrylring
Copy link
Author

I realized I should be using a type guard instead. There's no need to coerce anything because I always know what the type is.

@MadaraUchiha
Copy link

MadaraUchiha commented May 28, 2017

I disagree with closing this issue @RyanCavanaugh @kitsonk @DanielRosenwasser.

Here's a valid usecase:

Object.keys(whatever).filter(isNaN) // filter out numbered indices

That doesn't work because Object.keys() rightfully returns a string[]. Conversely, the following equally popular pattern is acceptable:

Object.keys(whatever).filter(Number) // filter out anything but numbered indices

Both of these patterns are extremely useful when iterating enums, for example, which have both keyed numbered indices.

Please reconsider, isNaN should at the very least accept strings.

P.S.
I know I can do something like

Object.keys(whatever).filter(k => isNaN(Number(k)));

But that unnecessarily ugly and verbose. This isn't Java.

@jcalz
Copy link
Contributor

jcalz commented May 28, 2017

Can't you overload the declaration of isNaN in your own code like this?

function isNaN(anything:any):boolean; // this doesn't emit anything

Object.keys(whatever).filter(isNaN);  // but this typechecks now

This gives you the behavior you want without imposing that decision onto those who prefer isNaN to accept only numbers. More generally, if you don't agree with a declaration in the standard library you could compile with a different one, right?

@RyanCavanaugh
Copy link
Member

After looking more at isNaN's results (which are complementary to isFinite), I'm extremely skeptical anyone using this for checking for safe numerical conversion understands what they're getting into. isNaN([ ]) returns false - do you really consider an empty array to be a number?

These functions return sensible values for things which are typeof n === "number" and manifest nonsense for anything else. The typings reflect that.

Number.isFinite and Number.isNaN behave differently and I could be convinced on those (but that's a separate issue)

@MadaraUchiha
Copy link

MadaraUchiha commented May 29, 2017

@RyanCavanaugh There are some extremely valid usecases for isNaN when the argument is a string. Iterating inputs and finding only the non-numeric values, as well as iterating an enum, which is a pure TypeScript concept that doesn't appear in JavaScript.

While I agree that isNaN does some crazy voodoo to get the argument to be a number (like your example with the array), it does make sense to be able to check the NaNness of strings specifically.

Also, your point with the array is irrelevant since it would still happen with direct conversion: isNaN(Number([])) // false

Can't you overload the declaration of isNaN in your own code like this?

I probably, could, I hadn't thought of that. But then again, I still maintain that it makes more sense for isNaN's accept type to be number | string, because it makes sense to use (and that's how it is being used) isNaN to check whether a string is not numeric in that way.

@darrylring
Copy link
Author

darrylring commented May 29, 2017

Fundamentally, isNaN and isFinite, operate on Number. JavaScript will happily silently coerce their inputs to Number for you. I believe TypeScript's current typings are correct.

@MadaraUchiha
Copy link

MadaraUchiha commented May 29, 2017

@darrylring No, that's not what isNaN is about. Number.isNaN operates on a number directly (and won't coerce). isNaN coercing the value it accepts into a number is a feature, not an unintended side effect.

Had I wanted to force only number values, I'd use Number.isNaN. And indeed there's no difference between isNaN(x) and Number.isNaN(Number(x)).

@jcalz
Copy link
Contributor

jcalz commented Jun 2, 2017

@MadaraUchiha, what do you expect the output of

var arr = ['','0B0','0E0','0O0','0X0','0.0','000','9E-999'];
console.log(arr.filter(isNaN));
console.log(arr.filter(Number));

to be?

@MadaraUchiha
Copy link

That's not entirely fair, given how all of those evaluate to 0, which is a falsey value. That's not the point being made here.

Note that again the protection that TypeScript gives you here is moot since arr.filter(x => Number.isNaN(Number(x))) would also bring me an empty array. An explicit cast isn't saving you here.

@jcalz
Copy link
Contributor

jcalz commented Jun 2, 2017

Your main use case is something like separating out the numeric and non-numeric keys of an object, right? I don't think isNaN or Number are the right functions for this since they have wacky edge cases. I'm not sure what the right functions are... maybe x => Number(x)+''!==x and x => Number(x)+''===x? These type-check by the way.

The function isNaN is confusing, and using it to identify "non-numeric strings" is probably a use-at-your-own-risk situation. If you want to do that in TypeScript, there's a small hoop to jump over (explicit cast, declaration overload, lambda expression, etc.). It doesn't look like anyone else thinks this use case is compelling enough to remove that hoop by default.

Side note: I don't think Number should type-check either since the callback to filter() should return a boolean. But I guess #5850 has been settled in the hoop-removal direction, since enough people want to use truthy/falsy values instead of booleans. Oh well.

This is all just my opinion, obviously. I was just trying to understand the use case.

@RyanCavanaugh
Copy link
Member

Added a FAQ entry

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants