Support dates #16
Support dates #16
Conversation
Closes yahoo#15
|
Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄 |
|
CLA signed. |
|
One caveat with: string that will match the ISO date format will be converted as Date, which might be an unexpected behavior (or just a breaking change). But I don't know how we can change this. |
| var DATE_LENGTH = new Date().toISOString().length | ||
| function isDate(d) { | ||
| try { | ||
| return ( |
There was a problem hiding this comment.
the try/catch might bring some slowness here, not sure.
There was a problem hiding this comment.
If we don't use the try/catch we must validate the date. It seems that d.getTime() will not throw, so we could test using isNaN().
There was a problem hiding this comment.
You can use isFinite() to see if it's a valid date without this try/catch.
|
CLA is valid! |
| @@ -25,13 +25,31 @@ var UNICODE_CHARS = { | |||
| '\u2029': '\\u2029' | |||
| }; | |||
|
|
|||
| // We can‘t just instanceof Date since dates are already converted to strings | |||
| // because of native Date.prototype.JSON (which use toISOString) | |||
| var DATE_LENGTH = new Date().toISOString().length | |||
There was a problem hiding this comment.
The toISOString() method returns a string in simplified extended ISO format (ISO 8601), which is always 24 characters long: YYYY-MM-DDTHH:mm:ss.sssZ.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString
Can you just set this to 24?
There was a problem hiding this comment.
Indeed, but this is more clear the way I coded it (imo) and it's computed only once per runtime, so no big deal (and clearer) imo.
There was a problem hiding this comment.
I disagree. I think setting it to 24 with a comment that it'll always be 24 is a better approach. But this is likely moot given my two comments above: #16 (comment)
|
Hello guys, |
|
@gchudnov The conversation here provides the current state of this PR, i.e. it's not currently merge-able. |
|
#27 has been merged. |
Closes #15
There is a little trick to test if a value is a date because
Date.prototype.toJSONis used before we can access the value in the stringify replacement function, so we end up with adate.toISOString()result.