Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

@almchung @outofambit Would you like to have a look at this? I would imagine including a check for NaN here as technically NaN is considered a number type, or even using the native isNaN or Number.isNaN function to do this check.

Good catch! The current isNumber() (which uses the native isNaN()) misses the case where Javascript returns the global property Infinity: L349. It's unclear whether Infinity can be considered a number, for the purpose of our drawing functions, we may only want to take finite number arguments. One remedy can be using isFinite()to only pass cases where finite numbers are passed as arguments. From MDN doc, isFinite() returns:

false if the argument is (or will be coerced to) positive or negative Infinity or NaN or undefined; otherwise, true.

In the case of NaN (not string that parses to NaN) it will satisfy the first condition of typeof NaN == "number" and return true right there. Directly using isNaN() only can take care of most cases I think, but there may be edge cases I didn't think of. For infinity we'll need to leave it be as it has some utility when dealing with number comparisons (max() and min() for example).

Sorry, I totally misread last time. I agree using isNaN() should solve this. Number.isNaN() doesn't count in the fact that a correctly formatted numerical string argument will be converted into a number.

Found another bug while testing: null argument would be converted to 0 and thus shouldn't throw EMPTY_VAR err msg.

I still think drawing functions like line() may benefit from using isFinite() or some kind of Infinity checking, but not sure if that should be a high priority :)

Is anyone currently working on this? If not I might have a crack at implementing the finite/infinite checking aspect

@oscarczer Please go ahead with a fix! Thanks.

Just confirming that we want null to be accepted as an input? I was going to implement an additional check here alongside finite checking to ensure that the string "null" isn't considered a valid parameter (as I don't believe that isNaN accounts for this) but saw what @almchung said and am now not so sure. I feel as if null and 0 shouldn't necessarily be treated the same as they could likely occur from different situations but if you'd like this to be the case I'll leave it as it :)

@oscarczer I agree that null and 0 shouldn't be treated the same. In some cases (like seen in line(), people will make use of this auto null-to-zero conversion, but this would be much rare (and since this kind of usage will be intentional, people can disregard the error messages).
I have a question though: in the proposed isNumber(), when param's type is string [here from PR], did you intend to check for an empty string, param=="" instead of param=="null"? I think it would be meaningful if isNumber() can catch the empty string case (especially since isNaN("null") would return true and isNaN("") would return false).

@almchung I was under the impression that isNaN(null) returns false as for some reason it is given the type of number (I think actually because of the instance it is able to be turned into 0?). I assumed that this would extent to isNaN("null") as well however I could definitely be wrong about that. Given this, it might also be worth putting a check for null in the first case of the param switch. However, irrespective of that I'm more than happy to add a check for isNaN("") as well as you're right about that also being an edge-case worth accounting for

Javascript can be confusing -- it would be helpful to check their String coercion rules on the official reference here.

I don't think we will need to add a separate case for checking null, since typeof(null)==object would follow default case where the current code already returns false.

Getting "null" or "" for your parameter would be a different case, where your typeof("null") and typeof("") are both string -- our current code would treat "null" as a string (so we get an expected output), however, the empty string will be converted into 0 (which is a special case we may want to include in our check).

Hopefully this is helpful!

Hi, I was wondering why we use isNaN() to check under the 'string' case in the function isNumber. From my understanding, isNaN should be under 'number' case.

@Bernice55231 For p5.js, it is possible to pass a number string (eg. "100") to functions expecting numbers and it'll work as if the string has been parsed into a string. isNaN() is special in that it works on both numbers and strings that can be parsed into numbers, and will return true if it found that a string passed in cannot be parsed into a number but false if the string can be normally parsed as a valid number (which is the behaviour matching p5.js).