Better checks for garbage input for setters

[wi-ski]

Added a helper for checking whether an argument is numeric. I feel as though its rather garish in implementation and would love some critiques if anyone feels this could/should be improved.

[mbad0la]

Hey @willdembinski

Thanking you for your PR. Could you have a look at my PR and let me know what was causing my code to fail the tests? It would help me understand the library better.

[wi-ski]

Yea no problem. I'm at work and don't have a lot of time. But - if you want to compare/contrast the behaviors of the PRs that would probably be the best way to learn something. Any suggestions/improvements you come up with in that process would be much appreciated as well. <3 :)

[maggiepint]

Is there any reason why simply using !isNaN() won't work?

[mbad0la]

That was exactly what I did, hence I wanted to know from @willdembinski himself.

[wi-ski]

The primary issue with isNaN() is how it handles [-]Infinity. I thought it would be good to avoid that as a possible input for a setter.

Also, there's little gems like these:

isNaN([1]) => false
isNaN([1,2]) => true

The take away is: Never underestimate the power of type coercion.

I think there was one other issue that I don't remember atm? I'll check when I get back home and get some more details.

[maggiepint]

I see your argument. I'll greenlight this, but I want a sanity check from @icambron or @ichernev .

[icambron]

+1. Infinity is a number so isNaN is right to exclude it, but it's not so useful in Moment. That array thing is just weird -- TIL.

[ichernev]

I hope to shorten it, this looks like hand-written and the wisdom of the internet should beat it. Not to mention, that this particular function should not be given floating point numbers, and isNumeric accepts them.

[ichernev]

We have things that are not numbers, and then for the numbers there are NaN, Infinity, floats and ints.

The Date setters, according to docs take integer argument, but if you try to use a string or array [1] it coerces and works, floats are rounded, for NaN/Infinity it sets invalid date.

So we use the part where strings are coerced (because we happily pass strings through), but we don't want to use the part where arrays are coerced? Also NaN/Infinity doing an invalid date seems fine to me.

Also, to top it off, your code does +val === +val in the form of _val === +val, that checks for NaN... in the most obscure way possible.

So why are we even doing this change? Coercion is part of JavaScript, its everywhere. If you want to guard like crazy 1) you're gonna have a hard time, and 2) everyone is going to hate you. I remember when we restricted some args to ints people complained that they couldn't pass strings now. The only thing that is questionable is the array coercion, which I didn't even now existed, but then -- everyone who takes out an integer for their purposes using the ~~val hack is going to achieve the same, so I don't see why bother. Put the check in your code, or everywhere in moment, but this just feels like the wrong overengineering.

[maggiepint]

@ichernev, have a look at #3483. This would fix that. If you like, we could simply go to !isNan(), but the behavior we're seeing in that issue definitely shouldn't be happening.

[wi-ski]

@ichernev - I agree with all the of points you outlined. In an effort to address the issues presented in #3483, I found that this approach handled the inputs that were causing failure.

Here's a snippet/series of cases if you find yourself investigating this further:
(Note: this is slightly different that what is the in the PR - would love to update if given the go-ahead)

function isNumeric(val) {
    var _val = +val;
    return (_val !== _val + 1) //infinity check
        && (_val === +val) //Cute coercion check
        && (typeof val !== 'object') //Array/object check
}
//Truethy
console.log(0,isNumeric("1"));
console.log(1,isNumeric(1e10));
console.log(2,isNumeric(1E10));
console.log(3,isNumeric("6e4"));
console.log(4,isNumeric("1.2222"));
console.log(5,isNumeric("-1.2222"));
console.log(6,isNumeric("-1.22220000000000"));
console.log(7,isNumeric("1.222200000000000000"));
console.log(8,isNumeric(1));
console.log(9,isNumeric(0));
console.log(10,isNumeric(-0));
console.log(11,isNumeric(1010010293029));
console.log(12,isNumeric(1.100393830000));
console.log(13,isNumeric(Math.LN2));
console.log(14,isNumeric(Math.PI));
console.log(15,isNumeric(5e10));
console.log(16,isNumeric(123));
console.log(17,isNumeric(123.123));
//Falsey
console.log(19,!isNumeric(NaN));
console.log(20,!isNumeric(Infinity));
console.log(21,!isNumeric(-Infinity));
console.log(22,!isNumeric());
console.log(23,!isNumeric(undefined));
console.log(24,!isNumeric('[1,2,3]'));
console.log(25,!isNumeric({a:1,b:2}));
console.log(26,!isNumeric(null));
console.log(27,!isNumeric([1]));
console.log(28,!isNumeric(new Date()));
console.log(29,!isNumeric('[1,2,3]'));

Additionally - here's some SO posts that I used when looking into the issue:

http://stackoverflow.com/questions/175739/is-there-a-built-in-way-in-javascript-to-check-if-a-string-is-a-valid-number/41458529#41458529

http://stackoverflow.com/questions/9716468/is-there-any-function-like-isnumeric-in-javascript-to-validate-numbers

If you feel that this unacceptable - regardless of circumstance - please let me know and I'll head back to the drawing board. Learned a decent amount setter behavior in the process and could use another go ;)

[wi-ski]

@icambron Is "Infinity" really a number tho? 😝 http://math.stackexchange.com/questions/36289/is-infinity-a-number

[ichernev]

Well, about the issue - maybe isValid should return false. Why add another layer of silent failures?

[wi-ski]

@ichernev - The argument against invalidating the moment object is really a matter of opinion - .add && .subtract fail silently while not invalidating the object.

var now = moment();

now.seconds(); => 52

now.add('teapot','seconds');

now.seconds(); => 52

I personally support the idea of invalidating all of the things. However, this is the behavior.

[ichernev]

@willdembinski that is a fair point. Even adding NaN is ignored, but setting something illegal to Date results in an Invalid Date.

If we do that I just want to make sure that we do it consistently everywhere, but I can't find more methods accepting user input (other than constructors). In 3.0 we can change this behavior.

[ichernev]

But lets use the logic from the add/subtract, which afaik is isNaN, and +val to make it int, so its really the same.

[wi-ski]

Ill do some tinkering on it - I think that using isNaN was the approach in a different PR that was failing cases for .day() and others, Ill check on it.

One thing to be aware of tho, isNaN() - for all intents and purposes that I can come up with atm - applies the degree of coercion you get when using the + operator before finally evaluating true/false.

I didn't believe this when I was first told either, however:

//Truethy
console.log(isNaN(NaN));
console.log(isNaN({}));
console.log(isNaN('foo'));

// Falsey
console.log(isNaN(1));
console.log(isNaN([]));
console.log(isNaN(null));
console.log(isNaN("1"));
console.log(isNaN(""));//<- https://imgflip.com/s/meme/Jackie-Chan-WTF.jpg

q = new Date() //=> Sat Jan 14 2017 18:11:13 GMT-0800 (PST)
isNaN(q) //=> false
isNaN(+q) //=> false

..etc

//Before you start thinking there might be a method to the madness...
[]+{}  //=>'[object Object]'
{}+[]  //=> 0 
0+{}  //=> '0[object Object]' //Pretty well known
{}+1  //=>1
1+[]   //=> '1'
[]+1   //=>'1'

I've been wrong before, so I'd love some input on this. But if it is true (that isNaN coerces before finally evaluating) then this:

function createAdder(direction, name) {
    return function (val, period) {
        var dur, tmp;
        //invert the arguments, but complain about it
        if (period !== null && !isNaN(+period)) {
            deprecateSimple(name, 'moment().' + name  + '(period, number) is deprecated. Please use moment().' + name + '(number, period). ' +
            'See http://momentjs.com/guides/#/warnings/add-inverted-param/ for more info.');
            tmp = val; val = period; period = tmp;
        }

        val = typeof val === 'string' ? +val : val;
        dur = createDuration(val, period);
        addSubtract(this, dur, direction);
        return this;
    };
}

Where if (period !== null && !isNaN(+period)) ... has a superfluous + operator.

But again, I'll do some tinkering and see what comes of it. 😃

[ichernev]

Coercion is fine, setting something obviously wrong and making the date invalid isnt. Next major we might rethink that.

Also if you do something other than isNaN change both places.

[wi-ski]

@ichernev - Apologies for going MIA. Work has required focus. #3735 uses the implementation we discussed. But the test cases that pass are not as robust as the previous implementation. Would be willing to discuss further unless you think its unnecessary.

Additionally - add/subtract uses the isNumber helper for validation in moment/src/lib/duration/create.js which doesn't play well with the types of values we should expect in the setter/getters. Unless Im misunderstanding something - but ideally we would use that helper in the get/set.

[ichernev]

@willdembinski so I'll merge the other one. About discussion about this one -- we already do some isNaN, isFinite checks elsewhere in the code. Doing 'hack' checks is no good unless you have a benchmark and a good reason. This PR is about fixing correctness, not speed.

About the correctness issue -- moment is horrible at that and we can't just "fix" it without breaking it also for existing users. We should have the same policy for all methods, and enforce it everywhere. This is for v3+, not now. For now we can just fix horrible things (like the issue you started fixing with this PR). I also want to make it better, just now is not the right time (v3 is).

[wi-ski]

@ichernev - I totally understand and sincerely appreciate the discussion. :) Let me know if there's anything else I can do on this one. In the mean time, on to the next one! :D

[ichernev]

Ok, closing this, we'll do something for v3 in this regard for sure.

[ichernev]

@wi-ski you can take a stab at this one, basing your change on the immutability branch in moment (v3). The idea is to catch all invalid mutations and produce an invalid moment.