andrewplummer / Sugar

A Javascript library for working with native objects.
https://sugarjs.com/
MIT License
4.53k stars 306 forks source link

Date parsing with month > 12 adds to the year instead of giving invalid date #636

Open ggoldsh opened 6 years ago

ggoldsh commented 6 years ago

Parsing 20/6/2018 gives an error, but parsing 19/6/2018 results in July 6, 2019. It seems 12 of the 19 months got converted to an extra year.

andrewplummer commented 6 years ago

Okay... first of all this is happening because anything that does not match Sugar's internal patterns falls back to native date parsing. I was going to go into how native JS sometimes allows overshooting dates, however it seems that this is limited to methods like setMonth and date parsing doesn't do this, so we can follow here.

I think we can consider this a bug and I'm going to have a look into tightening up parsing as a whole. This will give better feedback to users and allow the application to handle invalid input better. For the next major version I'm even going to look into not falling back to native parsing to avoid inconsistent results. In addition I'd like to also consider throwing errors on invalid input. I'll open a second ticket for that.

andrewplummer commented 6 years ago

Just created #637 and #638 for the issues I mentioned.

andrewplummer commented 6 years ago

Ah one other point I forgot to mention is that this is going to be a bit tricky :)

Month parsing is one thing but if we do this I'd like to do it across the board which means that February 29 parses only in leap years. This means I think that it also won't parse when no year is passed, as it is inferred to be the current year. This means that the code will have to be modified to do some post-hoc rejection as it won't know whether 29 is invalid or not until the year has been parsed.

Just a footnote of insanity here: Sugar is not timezone aware nor will it be for the foreseeable future (it plays well with timezone-js however). Just to note, the time 01:30AM may not be a valid time depending on your timezone, however preventing this input is outside the scope of Sugar and it will pass it through to setHours which will result in 02:30AM. This is at least consistent with native JS.

andrewplummer commented 6 years ago

Actually I take it back... it seems that native JS can allow overshooting in parsing... for example:

new Date('2/30/2018');

It seems that Chrome and FF only perform a naive check past 31. This isn't extremely consistent. Even though I've often said in the past Sugar conforming to JS specs is tantamount, the consistency of non-existing dates always parsing as invalid has value too.... will have a think about this one.

andrewplummer commented 6 years ago

Also note that any date format outside ISO8601 is outside the spec, so it's uncharted territory here.

andrewplummer commented 6 years ago

The other question this opens up is should the 29th fail because no month/year is passed and it happens to be a non-leap-year February at the moment? In terms of principles of not surprising the user this seems rather mysterious too.

andrewplummer commented 6 years ago

Sorry buddy I think you opened up Pandora's box here :)

My instinct is to keep the behavior here the same as the browser vendors, even if it's not guided by the spec. i.e. Parse all dates up to including 31, pushing them past the month edge if they parse. Your original issue of the months is still valid though and I've locked it down...browser vendors agree with you as well, fortunately.