Adding moment.fn.isValid #306

Merged
merged 7 commits into from Jun 8, 2012

Projects

None yet

3 participants

@timrwood
Member

Not ready to be pulled in yet, just wanted to open the discussion.

timrwood added some commits May 10, 2012
@timrwood timrwood First pass at isValid 776103b
@timrwood timrwood More work on isValid
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.
7fcf274
@timrwood timrwood Adding logic for handling whether the array was created with new Date…
…() or Date.utc()
4b65aa3
@timrwood
Member

Right now, ISO8601, array, and string from format(s) all run through the internal function dateFromArray(input, asUTC). See the function here

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 Date.parse(), it should also fail on the line with return !isNaN(this._d.getTime());

@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.

@rockymeza
Contributor

What is the use case for this again?

@timrwood
Member

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...");
}
@rockymeza

This should test a valid date, like 2010-01-01T24. Otherwise how do you know what it's failing on?

Member

Nice catch, I fixed it in a later commit. Additionally, there aren't any invalid fractional seconds as the range is from 0 - 999. I'm removing the test for fractional seconds failing.

@rockymeza

this test cannot have the same name as the previous test.

Member

Oops, copy paste error. Fixed in a later commit.

timrwood added some commits May 11, 2012
@timrwood timrwood Fixing duplicated unit test names 9a0e96d
@timrwood timrwood Fixing tests so that they use actual dates so we can test that hours …
…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.
3bf93a2
@timrwood timrwood Merge commit '618441a45f2937d9294e92f73d624f5a7d0ffe1a' into feature/…
…isValid
568bb72
This was referenced May 15, 2012
@timrwood
Member

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?

@nailor
nailor commented May 24, 2012

Looks good to me :)

@timrwood timrwood referenced this pull request May 29, 2012
Closed

1.7.0 Changelog #288

@rockymeza
Contributor

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?

@timrwood
Member

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 Invalid Date or by comparing the input array to the generated array after creation. Comparing array is used with moments created with an array as well as moments created with a string and format (including the automatic iso8601 parsing).

So the two ways to check for validity are to check when the moment is created, or check the first time isValid is called.

// 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.

@timrwood timrwood merged commit eaca7e6 into develop Jun 8, 2012
@timrwood
Member
timrwood commented Jun 8, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment