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

Weird parsing behavior for invalid dates with years before 1970 #2645

Closed
elreimundo opened this issue Oct 2, 2015 · 5 comments
Closed

Weird parsing behavior for invalid dates with years before 1970 #2645

elreimundo opened this issue Oct 2, 2015 · 5 comments
Labels

Comments

@elreimundo
Copy link

> moment([2015, null, null]).isValid()
< false
> moment([1968, null, null]).isValid()
< true

The second date is just as invalid as the first date. This appears to be a direct result of the pre-1970 workaround in https://github.com/moment/moment/blob/develop/src/lib/create/date-from-array.js.

@mattjohnsonpint
Copy link
Contributor

Closed in favor of #2646

ichernev pushed a commit that referenced this issue Nov 9, 2015
ichernev added a commit that referenced this issue Nov 9, 2015
@ichernev ichernev reopened this Dec 23, 2015
@ichernev
Copy link
Contributor

@elreimundo what is moment([1968, null, null]) supposed to mean? If you have a year only, you can use moment([1968]), this will default the rest to 1 (month, day, time to midnight). The current fix (or I would say hack) proposed in #2646 only works for the year. m = moment([1945, 5, null]) happily produces 1st Jan 1945 (because the null makes the whole thing invalid, and then we set the year but only to fix years in the range 0 - 100, not to fix invalid inputs.

If we ever want to make moment([1968, null, null]) and/or moment([2000, 5, null]) work we'd better cut the array after the first null and use that. And complain if there are any non-nulls after the first null. -- I do not want moment([2000, null, 5]) to be valid at all.

@elreimundo
Copy link
Author

@ichernev 1) it doesn't matter whether you agree with the validity of moment([1968, null, null]); the fact that there is a difference in the validity of moment([2015, null, null]) vs moment([1968, null, null]) is evidence of a bug in the source. 2) I wouldn't have opened the PR if I hadn't run into this very issue in production using separate dropdowns for year/month/day. If year has a value but month and day do not, moment behaves inconsistently for pre- and post-1970 dates.

My fix does not make moment([2015, null, null]) valid, and nor should it; rather, it brings pre-UTC-era dates in line with the behavior of post-1970 dates.

Also, what grounds do you have for calling "hacky" a legitimate check for isFinite and corresponding tests that verify that pre-1970 years and post-1970 years behave equivalently?

@ichernev
Copy link
Contributor

@elreimundo well, in your PR you mentioned that one date was valid and the other wasn't, and you made them both invalid (but I did't pay attention and thought they were both made valid). That is why I said "hack", because I though it "fixes" something that is not supposed to work.

I agree -- the behavior of the two should not be different. We changed the code in the last (unreleased) version to only setFullYear if the year is from 0 to 100, which will happen much less likely.

But then -- why don't we check if the Date object is valid, not if its year is finite. There can not be an infinite year in a valid date.

@ichernev
Copy link
Contributor

ichernev commented Aug 1, 2016

Fixed in #2646

@ichernev ichernev closed this as completed Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants