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

improves documentation for isDate() #480

Closed
wants to merge 1 commit into from
Closed

improves documentation for isDate() #480

wants to merge 1 commit into from

Conversation

tacomanator
Copy link

Was confused by the docs which said isDate takes a string. Took some time to figure out correct way. Hoping this helps others avoid that.

@chriso
Copy link
Collaborator

chriso commented Jan 26, 2016

It does take a string! The library validates strings only, and your dates are being coerced to a string before validation:

> new Date(2015, 1, 26).toString()
'Thu Feb 26 2015 00:00:00 GMT+0900 (JST)'

The date validator is fairly lenient which may be the cause of confusion (#476). Can you provide some examples that you found confusing?

@tacomanator
Copy link
Author

@chriso I see that I didn't investigate enough. My expectation was that isDate would accept any string that works with new Date(). However I ran into the following problem:

 > new Date('2011-12-21')
<· Tue Dec 20 2011 16:00:00 GMT-0800 (PST)

 > isDate('2011-12-21')
<· false

 > isDate(new Date('2011-12-21'))
<· true

While looking into it I found #476, which says to use toDate on strings before passing them to isDate. This worked for the above date, so I made a bad assumption.

 > isDate(toDate('2011-12-21'))
<· true

Is it supposed to be that isDate only accepts strings in the format output by date.toString()?

@chriso
Copy link
Collaborator

chriso commented Jan 27, 2016

As noted in #476, the isDate validator uses the built-in Date.parse() function which is very lenient. If Date.parse(str) returns something other than NaN then the validator attempts to filter out invalid dates such as 2015-02-29:

> validator.isDate('2011-12-21')
true
> validator.isDate('2015-02-29')
false

The fact that your validator.isDate('2011-12-21') is returning false is a bug, probably timezone related (#472, #431). What version are you using and what's the result of new Date().getTimezoneOffset() for you? Do any of the unit tests fail (npm test)?

@tacomanator
Copy link
Author

@chriso Version is 4.5.1

> new Date().getTimezoneOffset()
480

One test fails:

  77 passing (256ms)
  1 failing

  1) Validators should validate dates:
     Error: validator.isDate(2011-09-30) failed but should have passed
      at test/validators.js:19:23
      at Array.forEach (native)
      at test (test/validators.js:14:23)
      at Context.<anonymous> (test/validators.js:892:9)

The same input fails in the browser. Here's a screenshot of the inspector:

2016-01-26_21-38-49

@chriso
Copy link
Collaborator

chriso commented Jan 27, 2016

Thanks. Would you mind applying the following diff and the re-running the tests?

diff --git i/validator.js w/validator.js
index 900e718..cd3b777 100644
--- i/validator.js
+++ w/validator.js
@@ -522,7 +522,8 @@
         } else {
             timezone = iso8601Parts[21];
             if (!timezone) {
-                return null;
+                // if no hour/minute was provided, the date is GMT
+                return !iso8601Parts[12] ? 0 : null;
             }
             if (timezone === 'z' || timezone === 'Z') {
                 return 0;

The following two dates are both valid ISO8601 dates, but Date.parse() parses the first in the user's timezone and the second in GMT:

> new Date('2015-01-01 12:00')
Thu Jan 01 2015 12:00:00 GMT+0900 (JST)
> new Date('2015-01-01')
Thu Jan 01 2015 09:00:00 GMT+0900 (JST)

@tacomanator
Copy link
Author

@chriso tests pass with the patch

chriso added a commit that referenced this pull request Jan 28, 2016
The validator accepts ISO 8601 dates which include "%Y-%m-%d" and
"%Y-%m-%d %H:%M". The validator makes use of Date.parse() to parse
the date before checking if the date is invalid (e.g. 2015-02-29).
The built-in Date.parse() function interprets "%Y-%m-%d" dates in
GMT but "%Y-%m-%d %H:%M" in the user's timezone. This caused the
validator to apply an incorrect timezone offset which then caused
it to get out of sync with the built-in Date.parse() function.
Because of this mismatch, certain dates such as 2011-12-21 were
being marked as invalid when the user had a timezone offset < 0,
e.g. in PST, since the date would become 2011-12-20 after the
incorrect timezone offset was applied.

This fixes the function that determines the timezone in the date.
It now checks for the special "%Y-%m-%d" case and returns a
timezone offset of 0 (GMT).

cc #480
@chriso
Copy link
Collaborator

chriso commented Jan 28, 2016

Thanks. I've published the fix in 4.5.2. Did you have any other issues/inconsistencies with the validator?

@tacomanator
Copy link
Author

Thanks I'll grab the new version and let you know if I run into any other issues :)

@chriso
Copy link
Collaborator

chriso commented Jan 28, 2016

Awesome, thanks!

@chriso chriso closed this Jan 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants