-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Adding moment.fn.isValid #306
Conversation
I moved all calls to Date.utc.apply({}, array) into makeDateFromArray. I also noticed a bug where moment([2010, 0, 0]) would be converted to moment([2010, 0, 1]) and had to fix a bunch of unit tests that were dependent on this bug.
Right now, ISO8601, array, and string from format(s) all run through the internal function The way I intend to check date validity is by comparing the input array to an array constructed from the date getters. I might as well expose the method on the prototype as moment.fn.toArray. If the string was just parsed as a string like in @rockymeza and @nailor what do you guys think about this? I need to add some more unit tests before pulling this in for future regressions, but I think it's pretty solid. |
What is the use case for this again? |
This is to address #235. The main use case would be validating user input. var input = moment($('#datepicker').val(), 'MM-DD-YYYY');
if (input.isValid()) {
// do something
} else {
alert("Thats not a date silly...");
} |
…minutes and seconds can fail I think its impossible to have an invalid fractional second, as it can be from 0 - 999. Removing the test for it.
Note: This will not work after manipulating the moment as it compares the current array of date parts against the one that was used to create the date. var a = moment([2010, 0, 1]);
a.isValid(); // true
a.add('months', 1);
a.isValid(); // false Other than that, I think this is ready to merge in. @nailor, want to weigh in as you opened the issue that warranted this change? |
Looks good to me :) |
That it ceases to be valid after manipulation is a bit odd. But I think that the major use case is supposed to be right after instantiation, not after manipulation. We should document that, or maybe change the behavior after instantiation? |
The reason I wanted to avoid checking at instantiation was to avoid a performance hit on all moments. The way we determine if a moment is valid is by checking if the date is So the two ways to check for validity are to check when the moment is created, or check the first time // add to the instantiation
function Moment(date) {
...
// this is added to every moment() call
this.isValid = true;
if (isEqualArray(originalArray, this.toArray())) {
this.isValid = false;
}
}
// add on first call
function Moment(date) {
...
this.originalArray = originalArray;
}
Moment.prototype.isValid = function() {
if (isEqualArray(this.originalArray, this.toArray())) {
return false;
}
return true;
} I guess I should do a jsperf on the actual impact of checking during instantiation before making a decision though. I'm not sure of the use case where someone would want to check the validity of a moment after they have modified it. There isn't really any way to right now, as all manipulation methods can overflow, so there would never be an invalid date that was modified. |
Here are the jsperf results. http://jsperf.com/momentjs-validation If we make it so that isValid will work even after modifying the moment, it will cut performance in half for any moment created. |
Not ready to be pulled in yet, just wanted to open the discussion.