Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

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

Closed
HelderMagalhaes opened this issue Sep 17, 2014 · 14 comments
Labels

Comments

@HelderMagalhaes
Copy link

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:

  • Act as a reminder, while the Firefox bug is fixed
// Firefox misses the positive sign - see http://bugzil.la/730831
  • Maybe consider creating or providing a workaround to allow Firefox to use its native stringify instead of the script-based emulation
    • when date parsing is not being used or dates are within reasonable timespans, users are currently getting a performance hit without a (good) reason
  • Allow to cross-link both issues so that anyone interested can easily keep up
@ghost
Copy link

ghost commented Sep 17, 2014

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.

@ghost ghost added the bug label Sep 17, 2014
@bnjmnt4n
Copy link
Member

👍 Sounds good to me.

@HelderMagalhaes
Copy link
Author

+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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

bnjmnt4n added a commit that referenced this issue Oct 9, 2014
bnjmnt4n added a commit that referenced this issue Oct 12, 2014
bnjmnt4n added a commit that referenced this issue Nov 15, 2014
@bnjmnt4n
Copy link
Member

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
Copy link
Member

Ping? ;) @kitcambridge

@bnjmnt4n
Copy link
Member

bnjmnt4n commented Dec 9, 2014

Ping?

@ghost
Copy link

ghost commented Dec 9, 2014

@D10 Sorry I missed this. 😿 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 6453f0f.

@bnjmnt4n
Copy link
Member

bnjmnt4n commented Dec 9, 2014

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
Copy link

ghost commented Dec 9, 2014

@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. 😉 Good call!

@bnjmnt4n
Copy link
Member

@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
Copy link

ghost commented Dec 24, 2014

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

bnjmnt4n added a commit that referenced this issue Dec 24, 2014
bnjmnt4n added a commit that referenced this issue Dec 24, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants