Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#5726 closed enhancement (fixed)

dojo.toJson() doesn't serialize dates

Reported by: kriszyp Owned by: kzyp
Priority: high Milestone: 1.7
Component: General Version: 1.0
Keywords: json Cc: alex, toonetown, eugene
Blocked by: Blocking:

Description

Both ES4 (http://wiki.ecmascript.org/doku.php?id=proposals:json_encoding_and_decoding&s=json) and Douglas Crockford's JSON library serialize dates to ISO 8601 strings. Currently Dojo serializes a Date to {}, which does not seem that helpful. I think we should use the same format as the others.

Attachments (2)

json.diff (605 bytes) - added by kzyp 6 years ago.
json.js patch
MSApproachJsonDate.patch (1.1 KB) - added by 7twenty 5 years ago.
Another approach to serializing Date objects to JSON

Download all attachments as: .zip

Change History (35)

comment:1 Changed 6 years ago by peller

see #4970. If you could attach the files as patches against svn, that would be great.

comment:2 Changed 6 years ago by peller

the thing that's always bothered me about Crockford's solution, though, is that it's asymmetrical. You can't have a Date object in the return data, of course. It seems to lend itself more to a registry approach, where certain data can be tagged, perhaps by property naming conventions or explicit wiring. Furthermore, it's unclear whether the ISO format is intended to be more capable, like the one in dojo.date.stamp. Did that ever get resolved in ES4?

comment:3 Changed 6 years ago by kriszyp

Yes, it is unfortunate that the Date->ISO string is asymmetric, but it certainly isn't as asymmetric as serializing Date -> {}. It does require some knowledge or pattern matching to parse date strings back to Date objects. I am hoping to write a json parse/stringify augmenter that could help with this, being advised by JSON Schema, but for base functionality, I still believe it is best to follow Crockford's approach here. I am not sure what is meant by "more capable". I will attach my changes as diff/patches.

comment:4 Changed 6 years ago by peller

Kris, have you looked at dojo.date.stamp? It allows partial dates/times and is a compromise between the ISO spec and what's used in the W3C specs, with an eye towards performance and code size. It would be nice to reuse that functionality, if appropriate, and use that for an augmented json schema. Note that there used to be a 'registry' approach to the json code, but I guess that was abandoned. I'd prefer a pluggable approach rather than baking it in base.

comment:5 Changed 6 years ago by kriszyp

I have looked at dojo.date.stamp. Do you want an alternate serialization of Dates for json? Still hesitant to diverge from Crockford here. Or do you want the json serializer to call dojo.date.stamp.toISOString? dojo._base.json is a _base file, and dojo.data.stamp is not. Would you really want that dependency?
As far augmenting json parsing/serialization with json schema, I would certainly do it as a plugin (in dojox). I would use AOP to add functionality when desired (still thinking about this).

comment:6 Changed 6 years ago by peller

dojo.date.stamp is a superset of what Crockford has, and as you say, his proposal is not final and I don't think it was even finalized by ES4.

base code can't call core code, but if you had an augmentation in dojox or dojo core, it could certainly call it. It could also be a 'mixin' in dojo core (see dojo.fx for an example) Easier to move code into base than out.

comment:7 Changed 6 years ago by kriszyp

I think the fancy stuff, like date parsing should be an augmentation from dojox. However, basic Date serialization seems like it is currently broken (outputting {}), and should be fixed in base, and it seems like avoiding dependency issues with dojo.data.stamp and simply using the providing patch would be the easiest solution. Or are you concerned about the patch being too big?

comment:8 Changed 6 years ago by peller

  • Cc alex added

A little concerned about code size, but mostly arguing that an asymmetrical solution is potentially confusing and that it's best to solve in one place, not two. But I don't feel all that strongly about it.

Marking #4970 as a dup of this.

comment:9 Changed 6 years ago by bill

  • Component changed from General to Date
  • Milestone changed from 1.1 to 1.2
  • Owner changed from anonymous to peller

Move all milestone 1.1 tickets to 1.2, except for reopened tickets and tickets opened after 1.1RC1 was released.

comment:10 Changed 6 years ago by peller

  • Component changed from Date to General
  • Owner changed from peller to alex

I don't really own this. Alex probably needs to look at this and decide whether to resurrect AdapterRegistry or something of that sort.

Changed 6 years ago by kzyp

json.js patch

comment:11 Changed 6 years ago by kzyp

I attached a new patch. The patch reduces the code size of json.js, and provides at least a semi-coherent serialization of dates (still not symmetrical, but less assymetrical than it used to be).

comment:12 Changed 6 years ago by peller

I'm +1 on the code size change, -1 on the date. It should be ISO or nothing. Date.toString() is horrible and does implementation- (browser) specific things. I'd rather have something that causes an error than provide data that's inconsistent. Also, should it be it === null ?

comment:13 Changed 6 years ago by peller

nm, you meant ==

comment:14 Changed 6 years ago by kzyp

The code size improvement could be done without the Date change. The date change costs (approximate bytes increase after minification):

  1. Date -> {}: no change
  1. Date -> it.toString(): +18 bytes
  1. Date -> ISO String: +210 bytes
  1. Date -> "new Date(" + it.getTime() + ")": +61 bytes (this isn't valid JSON, but it is perfectly symmetric)

Any preference?

comment:15 Changed 6 years ago by peller

Option (e) is some sort of mixin to provide this functionality, like

if(dojo.date.stamp){
...
}

or do a mixin module, which is kind of weird, I know. Is there precedence for that?

comment:16 Changed 6 years ago by kzyp

Yes, you are right, options a,b, and d can be combined with option e "ISO if available":
if(dojo.date.stamp){

return dojo.date.stamp.toISOString(it);

}
It adds an additional ~60 more bytes, and it has different behavior based upon what modules are loaded.

comment:17 Changed 6 years ago by kzyp

Here is a variation on ISO that reduces the weight to 124 bytes (it is not perfectly ISO, it doesn't convert to double digit for single digit dates and months):
if(it instanceof Date){

return it.toUTCString().replace(/(.*?(\d+).*?(\d+)) (.*) \w*/,"$3-" + it.getUTCMonth() + "-$2T$4Z")

}

comment:18 Changed 6 years ago by dmachi

  • Type changed from defect to enhancement

comment:19 Changed 6 years ago by bill

  • Milestone changed from 1.2 to 1.3

bump enhancements to next milestone, as we prepare to close out 1.2

comment:20 Changed 5 years ago by bill

  • Milestone changed from 1.3 to future
  • Summary changed from Serializing Dates to JSON should in ISO 8601 format (UTC time) to dojo.toJson() doesn't serialize dates

Also #4970

comment:21 Changed 5 years ago by toonetown

  • Cc toonetown added

comment:22 Changed 5 years ago by elazutkin

  • Cc eugene added

comment:23 Changed 5 years ago by 7twenty

Microsoft uses an approach that is both symmetric and standard JSON:
http://weblogs.asp.net/bleroy/archive/2008/01/18/dates-and-json.aspx

I've created a patch that adds very little code and correctly round trips Date objects:

dojo.fromJson(dojo.toJson({date: new Date()}))

I've done minimal testing, but it worked correctly on FF 3.0.5, IE7, and Chrome.

Changed 5 years ago by 7twenty

Another approach to serializing Date objects to JSON

comment:24 Changed 5 years ago by toonetown

BTW - using that new approach to serialization adds 135 bytes (57 bytes gzipped) to base.

comment:25 Changed 5 years ago by peller

Yikes. People have been wrestling with this problem for a while and while this may be the closest I've seen to a type-specific solution in JSON, well, there are a couple of reasons not to follow MS here. One is that Crockford does have a solution offered in ES31, so this would diverge from the standard. (We should probably consider supporting stringify/parse) MS does seem to rely on a real hack and not be generalizable, and by not using ISO format, it invites more timezone problems, etc. I know it's unfortunate that the reviver/replacer system offered in ES31 ends up doing this more on a schema (particular named properties or property patterns) rather than a type, but that's because unfortunately JS doesn't have a native Date type, so it really just doesn't fit well into JSON to do it any other way.

comment:26 Changed 5 years ago by toonetown

Yeah - I don't figure that the patch will be good enough for inclusion into base any time soon - but at least it provides a solution for someone who it might be "good enough" for...and doesn't mind patching their own source.

However, I don't quite follow on the timezone issues that it brings...I thought that date.getTime() and new Date(seconds) are based off of GMT...

comment:27 Changed 5 years ago by peller

See also #8111. Matching these signatures would provide a solution the problem, though maybe not in a way that makes everyone happy.

@toonetown: well, one reason not to use the Unix epoch is because it assumes a particular time zone. If you want to represent a particular date, without regards to absolutes -- a common scenario -- you'll inevitably end up off by a day in some other timezone.

comment:28 Changed 5 years ago by toonetown

Hmm - also, this patch seems to cause performance issues when using xhrGet with handleAs of json if the payload is large...

comment:29 Changed 3 years ago by chrism

  • Owner changed from alex to dylan

please review/triage

comment:30 Changed 3 years ago by dylan

  • Milestone changed from future to 1.8
  • Owner changed from dylan to kriszyp

Decide what to do with this in 1.8, in time for 2.0.

comment:31 Changed 3 years ago by bill

  • Owner changed from kriszyp to kzyp

comment:32 Changed 3 years ago by kzyp

  • Resolution set to fixed
  • Status changed from new to closed

Fixed with latest checkin. Note, that there are some differences between how JSON.stringify(new Date(12)) works in browsers:
IE8: "1970-01-01T00:00:00Z"
WebKit & FF: "1970-01-01T00:00:00.012Z"
IE6-7 (our code): "1970-01-01T00:00:00.01Z"

comment:33 Changed 3 years ago by chrism

  • Milestone changed from 1.8 to 1.7
Note: See TracTickets for help on using tickets.