Moment.js overflows in parsing

[nailor]

When doing a parsing on a nonexistent date, moment "overflows", eg.

moment("2011-01-41")

is parsed as February 10th 2011. The date parser could probably throw an exception or indicate that there's been an error returning null/False or something else, that can't be mixed with a date.

[rockymeza]

Sometimes, it seems that having an overflow could be useful, #242. For example, to get the last day of the previous month, you could run:

moment().date(0);

This is not really an argument against throwing an error, but perhaps the overflow is more useful? @nailor, Is there a reason you need to fail from parsing an incorrect date?

[nailor]

Yes. Failing parsing incorrect date is useful for example if you want to detect if a date entered is incorrect.

[rockymeza]

In addition to failing on parse, do you think that moment should throw errors elsewhere? For example the date setter. If you give it something of a nonsensical value like -1, should it also throw an error?

I think that JavaScript's error handling is poor compared to other languages, like Python. It would have to look something like this if we used an exception.

try {
  moment('2011-01-14');
} catch (e) {
  if ( e instanceof moment.Exception ) {
    alert('failure');
  }
  // else, probably should re-raise the error;
}

I think that there definitely needs to be a parse exception. I had some errors with parsing in a project I was working on and I didn't realize that I had invalid dates because I still received a moment object. For example, the following code does not inform you that it failed:

var m = moment('asdf');
// { _d: Invalid Date, _isUTC: false }

@timrwood, thoughts? Is it viable to start throwing exceptions when people who have been using the library haven't been expecting exceptions?

[nailor]

Yes, I think the library should clearly throw an exception when it fails to parse things and not default to something obscure, like a moment object with some field set to some funny value.

Even though working with exceptions is bit more tedious in JS than in some other languages, it would still make sense to throw up the exceptions to avoid people breaking their software because of rather unexpected results.

What comes to changing behavior: bump up version number to new major and announce that the new version has background incompatible changes.

[timrwood]

It may be better to add a method to the prototype that checks validity.

moment.fn.isValid = function() {
    return !!this._d && !isNaN(this._d.getTime());
}

This won't throw errors when people aren't expecting them, and people can still check for errors if they dont trust the input data.

I think the parser should throw a moment into being invalid, as this matches native js behavior.

console.log(new Date('2011-0-100')); //Invalid Date

However, all the Date.prototype.get/set functions support overflowing, and it's actually pretty useful to support it, so I don't think that should be invalid behavior.

[nailor]

Proposed validation method would not work in this case. For example:

moment("2011-01-40").isValid() 

would return true, as it get's parsed to 10th of February.

I agree that overflowing is useful, but I'd like to highlight the original problem: Parsing invalid date as a valid in a way, that you really can't be sure (unless, say, you convert it back to expected format and compare). Building a check in parsing that the given date is valid is not too difficult, it can, in the beginning, even be implemented with the naive check of converting date back to the same format and seeing if match the original.

What comes to mimicking behavior of the Date object, I think that if user would like to have that kind of faulty behavior, he'd use the Date directly. Currently the ISO 8861 timestamp parsing for Date is available only in JS 1.8.5, before that it only supported RFC2822 style dates.

So, if there will be a date parsing in moment.js, why not make it notify about the date validity when parsing? Overflowing would be still possible by passing a array of integers to the moment contsructor.

[timrwood]

Sorry, yes, this is what I was referring to with this line.

I think the parser should throw a moment into being invalid, as this matches native js behavior.

There should be a check for validity while parsing moment("2011-01-40"), and then just add a flag stating that the moment is invalid.

moment.fn.isValid = function() {
    return !!this._isValid && !!this._d && !isNaN(this._d.getTime());
}

[timrwood]

Check out the progress on moment.fn.isValid in #306