bestiejs / json3

A JSON polyfill. No longer maintained.
https://bestiejs.github.io/json3
Other
1.01k stars 150 forks source link

Native stringify is not being used (at all!) in Firefox due to a (Fx) bug in date serialization #65

Closed HelderMagalhaes closed 9 years ago

HelderMagalhaes commented 10 years ago

Due to Firefox bug 730831, Firefox is currently causing the date test to fail and, therefore, always triggering the script-based stringify instead of the native implementation.

Although this was already triaged to be a Firefox issue, this was created in order to:

// Firefox misses the positive sign - see http://bugzil.la/730831
ghost commented 10 years ago

Ouch; thanks for bringing this up! We can probably use a custom replacement function to serialize extended dates if the JSON.stringify(new Date(8.64e15)) == '"+275760-09-13T00:00:00.000Z"' test fails.

bnjmnt4n commented 10 years ago

:+1: Sounds good to me.

HelderMagalhaes commented 10 years ago

+1 from here too.

I'd highlight the believe that not using the native stringify in Firefox at all currently seems a high cost to pay for such a subtle - although legitimate and already identified/reported - issue and likely rarely used feature - serializing dates in ISO format is somehow recent, at least as a standard; also extended dates seem to be used in highly specific circumstances (like in "Ship's Log, Stardate 52152.6. [...]", still I'm throwing a "less than 1%" of users/use-cases).

I'd even hint towards using the custom function in case any of the extended date tests fail: that would further help covering a few few more broken implementation, guessing from the documentation.

HelderMagalhaes commented 10 years ago

A few tests showed that, in order to workaround the current behavior (but stop being extended date compliant - use with care!) for current Firefox versions (tested with 32.01 and 35.0a1), one may disable (comment out) the following lines:

stringify(new Date(8.64e15)) == '"+275760-09-13T00:00:00.000Z"' &&
[...]
stringify(new Date(-621987552e5)) == '"-000001-01-01T00:00:00.000Z"' &&

For disabling all extended date checks and, as stated in previous comment, further broaden support for other broken browser versions when extended date support is not in use, then the following lines may be disabled as well:

stringify(new Date(-8.64e15)) == '"-271821-04-20T00:00:00.000Z"' &&
[...]
stringify(new Date(-1)) == '"1969-12-31T23:59:59.999Z"';

It might be nice, IMO, to have some sort of preference for disabling extended date support, as it involves a few operations during start-up and also seems to hold a minor extra overhead during serialization/interpretation.

bnjmnt4n commented 10 years ago

Thanks for the report, I’ll work on this in a few weeks’ time (currently busy with exams). What I think we’ll do is check for date support separately, since that has many bugs across browsers. If the environment has buggy date serialisation support, but an otherwise working native JSON.stringify implementation, we’ll use the native implementation with a callback to ensure proper date serialisation.

HelderMagalhaes commented 10 years ago

@d10: Sounds like a a good approach, IMO!

bnjmnt4n commented 10 years ago

After doing some thinking, I've realized that using a custom replacement function is not going to work. If the user specifies an array of property names to stringify, it would be impossible to use a replacement function while allowing filtering of properties from the array.

The only solution left would be to overwrite the Date#toJSON method. Thoughts? /cc @kitcambridge @HelderMagalhaes

bnjmnt4n commented 10 years ago

Ping? ;) @kitcambridge

bnjmnt4n commented 9 years ago

Ping?

ghost commented 9 years ago

@d10 Sorry I missed this. :crying_cat_face: You're right; we'd need to override Date#toJSON...we can come close by providing a custom wrapper function, but not for non-enumerable properties. Seems like a logical follow-up to 6453f0f840a9ca2444e0eb61bde57d64633fc1c6.

bnjmnt4n commented 9 years ago

we can come close by providing a custom wrapper function, but not for non-enumerable properties.

@kitcambridge not too sure what you mean here.

ghost commented 9 years ago

@d10 I meant we could have our stringify implementation wrap nativeStringify, passing a replacement function containing this logic. If the user-specified filter argument is a function, our replacement function would invoke it; otherwise, we check if filter contains the property passed to our replacer. But, if filter contains non-enumerable properties, our replacement function won't be called.

I think another alternative is to recursively copy source (serializing any dates we encounter), and passing the copy to nativeStringify. That seems expensive, though, and we'd still need all the circular reference machinery.

TL;DR: Overriding Date#toJSON is the way to go. :wink: Good call!

bnjmnt4n commented 9 years ago

@kitcambridge I’m quite busy now, do you think you could finish up the Date#toJSON fix? OT: for the 4.0 release, I think we should write release a blog post, I’ve got a rough draft of the content, will ping you when it’s done. :)

ghost commented 9 years ago

@d10 Sure thing; I've been really busy, too. :smile: I'll try to squeeze it in before the new year; otherwise, 4.0 will be our first 2015 release.