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

Prototype <= 1.6.1 Support #8

Closed
kitcambridge opened this Issue Apr 28, 2012 · 6 comments

Comments

4 participants
@kitcambridge
Member

kitcambridge commented Apr 28, 2012

So...here's the story behind JSON 3 not supporting Prototype <= 1.6.1.

According to the spec entry for stringify, the toJSON method is used to obtain an alternative object for serialization. For example, Backbone's Model#toJSON returns a plain object containing the model's attributes, which is then serialized using the algorithm defined in the spec. For this behavior to make sense, toJSON should return a string, number, Boolean, array literal, or object literal—the only objects for which the serialization routine is explicitly defined.

Unfortunately, older versions of Prototype dictate that the toJSON method should return a pre-serialized JSON string. To make matters worse, it also defines toJSON methods on strings, dates, numbers, and arrays (but not booleans)—which means that stringify will call them to obtain a replacement value (a pre-serialized string), and then serialize that value. The result will always be a string literal, rather than a proper JSON structure.

This behavior is actually consistent with the spec, and using the native implementation will yield identical results. We can confirm this by running the following snippet in a browser that supports native JSON, and without JSON 3 loaded:

(function() {
  var script = document.createElement("script");
  script.src = "http://prototypejs.org/assets/2009/8/31/prototype.js";
  script.onload = function () {
    // This should be `true`.
    alert(JSON.stringify([1, 2, 3]) == "\"[1, 2, 3]\"");
  };
  document.body.appendChild(script);
}());

The spec doesn't account for a library that completely redefines how toJSON works, and catering to these versions of Prototype would require breaking compatibility with the spec. The objective of JSON 3 is twofold: provide a spec-complaint polyfill for older browsers, and fix bugs in the native JSON implementations of newer ones. I'm afraid that fixing a buggy, dated library (1.6.1 was released in September 2009; 1.7, which corrects the issues, was released in November 2010) would fall outside the scope of this project.

A Note on Object.toJSON: The Object.toJSON method provided by Prototype is not an appropriate fallback for JSON.stringify—it doesn't support the filter or whitespace arguments, and will recurse indefinitely when attempting to serialize cyclic structures. Additionally, Prototype's date serialization is broken (it doesn't support extended years or milliseconds, for example), and doesn't work around the bugs present in Opera >= 10.53.

The only viable option that I can see is to monkey-patch Prototype. There are two significant problems with this approach as well. First, it would involve increasing the size and complexity of the source for a very specific audience. Secondly, even if all of Prototype's buggy methods could be monkey-patched, nothing can be done about third-party code. Remember that Prototype redefines the behavior of toJSON—which means that third-party code that relies on this behavior will break if the library is patched. It's a less than stellar solution.

I'm certainly open to discussing this, but let's keep in mind that any solution we come up with is likely to be an egregious hack. For that reason, I'd like to tread cautiously before implementing any kind of a patch (@tobie, @savetheclocktower—if you have the time and inclination, I'd love to hear any thoughts that you may have about this).

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Apr 28, 2012

Ya, here's the backstory to this unfortunate issue: https://groups.google.com/group/prototype-core/msg/76508c7dae8be2a1

Nothing much to be done. Prototype <= 1.6.1 was designed around Crockford's initial JSON implementation which differed completely.

tobie commented Apr 28, 2012

Ya, here's the backstory to this unfortunate issue: https://groups.google.com/group/prototype-core/msg/76508c7dae8be2a1

Nothing much to be done. Prototype <= 1.6.1 was designed around Crockford's initial JSON implementation which differed completely.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 30, 2012

Member

I've had libs that are loaded in pages first and handle the Prototype issue by doing something like

;(function(window) {
  var JSON = {
    'parse': window.JSON.parse,
    'stringify': window.JSON.stringify
  };
  // ...
  function stringify(value, replacer, space) {
    // backup `toJSON` methods
    var key = 'toJSON';
    var backups = [
      [Array,   hasOwnProperty.call(ArrayProto, key) && isFunction(ArrayProto[key]) && ArrayProto[key]],
      [Boolean, hasOwnProperty.call(BoolProto, key)  && isFunction(BoolProto[key])  && BoolProto[key]],
      [Object,  hasOwnProperty.call(ObjProto, key)   && isFunction(ObjProto[key])   && ObjProto[key]],
      [Number,  hasOwnProperty.call(NumProto, key)   && isFunction(NumProto[key])   && NumProto[key]],
      [String,  hasOwnProperty.call(StrProto, key)   && isFunction(StrProto[key])   && StrProto[key]]
    ];
    // delete `toJSON` methods 
    forEach(backups, function(pair){
      pair[1] && delete pair[0].prototype[key];
    });
    // get stringify result
    var result = JSON.stringify(value, replacer, space);
    // restore `toJSON` methods
    forEach(backups, function(pair){
      pair[1] && (pair[0].prototype[key] = pair[1]);
    });
    return result;
  }
  // ...
}(this));

That said, I don't think its JSON3's problem to fix an old lib's bad code.
The devs using Prototype should be well aware by now of its issues/limitations.

Member

jdalton commented Apr 30, 2012

I've had libs that are loaded in pages first and handle the Prototype issue by doing something like

;(function(window) {
  var JSON = {
    'parse': window.JSON.parse,
    'stringify': window.JSON.stringify
  };
  // ...
  function stringify(value, replacer, space) {
    // backup `toJSON` methods
    var key = 'toJSON';
    var backups = [
      [Array,   hasOwnProperty.call(ArrayProto, key) && isFunction(ArrayProto[key]) && ArrayProto[key]],
      [Boolean, hasOwnProperty.call(BoolProto, key)  && isFunction(BoolProto[key])  && BoolProto[key]],
      [Object,  hasOwnProperty.call(ObjProto, key)   && isFunction(ObjProto[key])   && ObjProto[key]],
      [Number,  hasOwnProperty.call(NumProto, key)   && isFunction(NumProto[key])   && NumProto[key]],
      [String,  hasOwnProperty.call(StrProto, key)   && isFunction(StrProto[key])   && StrProto[key]]
    ];
    // delete `toJSON` methods 
    forEach(backups, function(pair){
      pair[1] && delete pair[0].prototype[key];
    });
    // get stringify result
    var result = JSON.stringify(value, replacer, space);
    // restore `toJSON` methods
    forEach(backups, function(pair){
      pair[1] && (pair[0].prototype[key] = pair[1]);
    });
    return result;
  }
  // ...
}(this));

That said, I don't think its JSON3's problem to fix an old lib's bad code.
The devs using Prototype should be well aware by now of its issues/limitations.

@kitcambridge

This comment has been minimized.

Show comment
Hide comment
@kitcambridge

kitcambridge May 6, 2012

Member

@tobie Thanks very much for chiming in. I've archived your Google Groups post as a Gist for future reference, and added a link to the GitHub Page.

@jdalton That's a great patch—might I suggest surrounding the call to JSON.stringify with a try...catch, though? That way, the toJSON methods will be restored even if stringify throws an error. It doesn't address third-party code (I'm not sure if that's even possible), but should be more than adequate for most circumstances.

I definitely agree that this doesn't belong in the core. The fact that the native implementation produces identical results is reason enough to keep the current behavior. Users of Prototype <= 1.6.1 can continue to use Object.toJSON and String#evalJSON—no need for JSON 3.

Member

kitcambridge commented May 6, 2012

@tobie Thanks very much for chiming in. I've archived your Google Groups post as a Gist for future reference, and added a link to the GitHub Page.

@jdalton That's a great patch—might I suggest surrounding the call to JSON.stringify with a try...catch, though? That way, the toJSON methods will be restored even if stringify throws an error. It doesn't address third-party code (I'm not sure if that's even possible), but should be more than adequate for most circumstances.

I definitely agree that this doesn't belong in the core. The fact that the native implementation produces identical results is reason enough to keep the current behavior. Users of Prototype <= 1.6.1 can continue to use Object.toJSON and String#evalJSON—no need for JSON 3.

@kitcambridge

This comment has been minimized.

Show comment
Hide comment
@kitcambridge

kitcambridge Jun 4, 2012

Member

I've implemented a simple fix for this in a326fcd. stringify will now ignore inherited toJSON methods on numbers, strings, dates, and arrays. It's far from perfect, but should take care of the majority of compatibility issues.

Pinging @oyvindkinsey—does this look okay to you?

Member

kitcambridge commented Jun 4, 2012

I've implemented a simple fix for this in a326fcd. stringify will now ignore inherited toJSON methods on numbers, strings, dates, and arrays. It's far from perfect, but should take care of the majority of compatibility issues.

Pinging @oyvindkinsey—does this look okay to you?

@oyvindkinsey

This comment has been minimized.

Show comment
Hide comment
@oyvindkinsey

oyvindkinsey Jun 5, 2012

Hey @tobie, cool to see you here :)

@kitcambridge, yeah - this seems to be logically consistent - thanks for doing this!

oyvindkinsey commented Jun 5, 2012

Hey @tobie, cool to see you here :)

@kitcambridge, yeah - this seems to be logically consistent - thanks for doing this!

@kitcambridge

This comment has been minimized.

Show comment
Hide comment
@kitcambridge

kitcambridge Jun 5, 2012

Member

@oyvindkinsey Thank you for the review!

Member

kitcambridge commented Jun 5, 2012

@oyvindkinsey Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment