JSON3 obliterates existing polyfills on window.JSON even when loading as an AMD module

[jaxley]

JSON3 (3.2.4) wraps itself in a self-executing function so that simply by loading the js file, it will execute. That might be okay if it didn't have side-effects, but it turns out that just by bundling the code in another AMD module, it will execute the self-invoking function in json3 and that executes the has() checks and determines in some cases that the existing parser (e.g. json2) isn't standards compliant and so it will replace the stringify and parse functions with JSON3! Even before loading the module itself!. I think that the only thing that the module should do in a self-invoking function is set up the define() invocation so that on file load, we will have json3 registered as a module to possibly load, but not diddle with window.JSON functions just by parsing the code. That should require actually loading the code in a module context.

I think the define should return a factory that will invoke the logic with side-effects and not invoke that logic on loading (in an AMD context).

Here's what I tweaked locally that illustrates probably a safer pattern:

pull all of the AMD detection logic and definition out into a self-invoking function:

(function() {
      // Detect the `define` function exposed by asynchronous module loaders. The
  // strict `define` check is necessary for compatibility with `r.js`.
  var isLoader = typeof define === "function" && define.amd, JSON3 = !isLoader && typeof exports == "object" && exports;

  if (JSON3 || isLoader) {
    if (typeof JSON == "object" && JSON) {
      // Delegate to the native `stringify` and `parse` implementations in
      // asynchronous module loaders and CommonJS environments.
      if (isLoader) {
        JSON3 = JSON;
      } else {
        JSON3.stringify = JSON.stringify;
        JSON3.parse = JSON.parse;
      }
    } else if (isLoader) {
      JSON3 = this.JSON = {};
    }
  } else {
    // Export for web browsers and JavaScript engines.
    JSON3 = this.JSON || (this.JSON = {});
    json3init();    // call the init code with side-effects here
  }

  // Export for asynchronous module loaders.
  if (isLoader) {
    define(function () {
      json3init();
      return JSON3;
    });
  }
})();

And remove the self-invoking function and instead assign this to a named function that can be called at the right time. This is not unlike a UMDjs factory pattern https://github.com/umdjs/umd

var json3init = (function() {
...
    // all of the other json3 code goes here
});

It would be cleaner if the code removed the implicit global JSON3 variable diddling and changed to a real factory method that returns an instance instead of initializing an implicit global var. you could then use the return value to assign to window.JSON3 only when warranted and avoid polluting global scope if not desired.

[jeffrose]

+1

[ghost]

Hi @jaxley,

This was actually the default behavior before v3.2.3, but was changed after a lengthy discussion in #14. The problem with reverting to the pre-3.2.3 behavior is this: if RequireJS, curl, or another AMD loader is loaded in conjunction with a third-party script (say, Facebook Connect, which includes JSON 3), the polyfill won't be loaded unless it's specified as a dependency in a define call. As @jdalton explains:

This [the current behavior] is more for the uncontrolled use like when JSON3 is compiled into a widget that's included on a page that happens to also use AMD (so JSON3 used as a global and as an AMD module).

Libraries like jQuery and Lo-Dash mitigate this by exporting a global even if an AMD loader is present. Both libraries also provide a noConflict function, so the global can be removed if desired. Unfortunately, because JSON 3 is a polyfill that can extend the native JSON object, I'm wary of providing similar functionality—JSON.noConflict isn't a standard method. Another solution would be to expose a global JSON3 object, perhaps with an install or runInContext method. What do you think?

Edit: kriskowal/es5-shim#167 is a compelling argument for why deferred execution with a module loader is undesirable.

[bnjmnt4n]

Closed via c05062. See #50.