-
Notifications
You must be signed in to change notification settings - Fork 187
Support dates #16
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
Support dates #16
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ var isRegExp = require('util').isRegExp; | |
|
||
// Generate an internal UID to make the regexp pattern harder to guess. | ||
var UID = Math.floor(Math.random() * 0x10000000000).toString(16); | ||
var PLACE_HOLDER_REGEXP = new RegExp('"@__(FUNCTION|REGEXP)-' + UID + '-(\\d+)__@"', 'g'); | ||
var PLACE_HOLDER_REGEXP = new RegExp('"@__(FUNCTION|REGEXP|DATE)-' + UID + '-(\\d+)__@"', 'g'); | ||
|
||
var IS_NATIVE_CODE_REGEXP = /\{\s*\[native code\]\s*\}/g; | ||
var UNSAFE_CHARS_REGEXP = /[<>\/\u2028\u2029]/g; | ||
|
@@ -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 | ||
function isDate(d) { | ||
try { | ||
return ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the try/catch might bring some slowness here, not sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't use the try/catch we must validate the date. It seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will try that. |
||
// testing length first to avoid new Date() for every string | ||
d.length === DATE_LENGTH && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this detection process seems to be very flaky, specially because Date() constructor is really flexible. e.g.:
Maybe you should use a regexp to detect, then call Date() on the value to validate, and if it throw, you should probably prevent swallowing the error, otherwise this will be crazy to debug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The date is automatically converted to ISOString by Date.prototype.toJSON before this code is executed. So this is not a problem. That's why I tested the length, because all date will be already converted to the same format at this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand, I'm just saying that you should probably apply a regexp to prevent other arbitrary strings with the same length to be treated as dates just because they have the exact same length. Otherwise an error will throw. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, the part that I don't like here is the comparison with DATE_LENGTH as the solely way to detect serialized dates. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for performance reason, we should avoid regex. That will be very expensive to use regex on all strings without a fast and easy way to detect non date string. Obviously, we need to ensure a date is valid. That's why I mentionned using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another approach: let currentObj = obj;
JSON.stringify(obj, function (key, value) {
// Do other value type checks...
if (typeof value === 'string' && currentObj[key] instanceof Date) {
return '@__DATE-' + UID + '-' + (dates.push(value) - 1) + '__@';
}
if (value && typeof value === 'object') {
currentObj = value;
}
return value;
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this one better, I forgot that we can check back into the original value if we want to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Are you sure about that? How does it works for key in depth?! (eg: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a depth-first traversal so I don't see why it wouldn't work: var obj = {
foo: {
bar: 'bar'
},
baz: {
date: new Date(0)
}
};
var currentObj = obj;
var output = JSON.stringify(obj, function (key, value) {
console.log(key);
if (typeof value === 'string' && currentObj[key] instanceof Date) {
return 'DATE!';
}
if (value && typeof value === 'object') {
currentObj = value;
}
return value;
});
console.log(output);
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I missed the assignation after the test. So yeah that might work :) |
||
d === ((new Date(d)).toISOString()) | ||
) | ||
} | ||
// Invalid Date might throw "RangeError: invalid date" when calling toISOString | ||
catch(e) {} | ||
|
||
return false | ||
} | ||
|
||
module.exports = function serialize(obj, space) { | ||
var functions = []; | ||
var regexps = []; | ||
var dates = []; | ||
var str; | ||
|
||
// Creates a JSON string representation of the object and uses placeholders | ||
// for functions and regexps (identified by index) which are later | ||
// for functions, regexps and dates (identified by index) which are later | ||
// replaced. | ||
str = JSON.stringify(obj, function (key, value) { | ||
if (typeof value === 'function') { | ||
|
@@ -42,6 +60,10 @@ module.exports = function serialize(obj, space) { | |
return '@__REGEXP-' + UID + '-' + (regexps.push(value) - 1) + '__@'; | ||
} | ||
|
||
if (typeof value === 'string' && isDate(value)) { | ||
return '@__DATE-' + UID + '-' + (dates.push(value) - 1) + '__@'; | ||
} | ||
|
||
return value; | ||
}, space); | ||
|
||
|
@@ -58,14 +80,22 @@ module.exports = function serialize(obj, space) { | |
return UNICODE_CHARS[unsafeChar]; | ||
}); | ||
|
||
if (functions.length === 0 && regexps.length === 0) { | ||
if ( | ||
functions.length === 0 && | ||
regexps.length === 0 && | ||
dates.length === 0 | ||
) { | ||
return str; | ||
} | ||
|
||
// Replaces all occurrences of function and regexp placeholders in the JSON | ||
// string with their string representations. If the original value can not | ||
// be found, then `undefined` is used. | ||
// Replaces all occurrences of function, regexp and date placeholders in the | ||
// JSON string with their string representations. | ||
// If the original value can not be found, then `undefined` is used. | ||
return str.replace(PLACE_HOLDER_REGEXP, function (match, type, valueIndex) { | ||
if (type === 'DATE') { | ||
return "new Date(\"" + dates[valueIndex] + "\")"; | ||
} | ||
|
||
if (type === 'REGEXP') { | ||
return regexps[valueIndex].toString(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. I think setting it to
24
with a comment that it'll always be24
is a better approach. But this is likely moot given my two comments above: #16 (comment)