@NekR opened this Issue on August 31st 2014

Here:
https://github.com/piwik/piwik/blob/master/js/piwik.js#L45

You check for JSON2 object in window object which never exists and you always use JS version of JSON instead of builtin.

Also, here https://github.com/piwik/piwik/pull/6066#issuecomment-53999069 man said that you do not use json2.js, but you actually do, or at least json.js which is worse.

Refer to this commend for more details: https://github.com/piwik/piwik/pull/6066#issuecomment-53997506

@mattab commented on August 31st 2014 Owner
@NekR commented on August 31st 2014

what is JSON2? any link to standard?

@mattab commented on August 31st 2014 Owner

@NekR
This place is our issue tracker, to keep track of bugs and feature requests. Please can you ask questions in the forums: http://forum.piwik.org/

@tsteur commented on August 31st 2014 Owner

We could actually indeed use builtin JSON API in case it exists.

JSON2 is a lib that we use as JSON API it is not supported by all browser see http://caniuse.com/#search=json

@halfdan commented on August 31st 2014 Member

@NekR It's a polyfill/fix for broken JSON implementations in some older browsers.

@mattab @NekR has a valid point. Checking for JSON2 on the window object hardly makes any sense as we define JSON2.stringify and JSON2.parse later in the js file.

@NekR commented on August 31st 2014

I knew what json2 is a pollyfill, but almost all browsers have builtin version.

So it's better to have

(function () {
    'use strict';

    if (window.JSON) {
        window.JSON2 = window.JSON;
        return;
    } else {
        window.JSON2 = {};
    }
   // ...

This is fast example how it should be. Or you can just replace everywhere word 'JSON2' to 'JSON'.

@tsteur commented on August 31st 2014 Owner

makes sense.

We could even think about removing JSON2 at some point completely. I just had a look and the browsers that do not support JSON builtin are less than 0.5% (IE7 has about 0.2%) and JSON is only needed when custom variables in scope visit are used anyway.

@mattab commented on September 1st 2014 Owner

@halfdan you reopened the issue, what do you think should be the scope? maybe Remove JSON2 from piwik.js or so?

@halfdan commented on September 1st 2014 Member

@mattab @NekR has shown an alternative solution that I find reasonable. We can also think about removing JSON2 completely if we decide that those 0.5% that @tsteur mentioned are not worth tracking anymore.

@mattab commented on September 1st 2014 Owner

@halfdan could you rename ticket to clarify scope? then feel free to assign to Short term if it's a quick fix

@unwiredbrain commented on October 15th 2014

How about using JSON 3 instead?

JSON 3 [...] is a drop-in replacement for JSON 2. [...] The JSON 3 parser does not use eval or regular expressions. This provides security and performance benefits in obsolete and mobile environments, where the margin is particularly significant.

The functions behave exactly as described in the ECMAScript spec, except for the date serialization [...] it does not define Date#toISOString() or Date#toJSON(). This preserves CommonJS compatibility and avoids polluting native prototypes. Instead, date serialization is performed internally by the stringify() implementation: if a date object does not define a custom toJSON() method, it is serialized as a simplified ISO 8601 date-time string.
-- http://bestiejs.github.io/json3/

Also, this will probably fix #5960.

@tsteur commented on October 15th 2014 Owner

Looking good to me. The minified version with 8kb is a bit a lot but should be still ok. Could go with this one for like a year and then support only builtin JSON.

Alternatively now or like in one year we would simply ignore browsers that do not support builtin JSON. If someone still wants those browsers tracked we could let it up to the user to load a separate file before that provides JSON2 or JSON3. So we would check if there is JSON, if not JSON2, then JSON3. If none is there we do not track on this very outdated browsers.

@mattab commented on October 4th 2016 Owner

Increasing priority as we probably soon could remove JSON2 from piwik.js

@tsteur commented on October 4th 2016 Owner

It's currently pretty much only there for IE7 & IE6 see http://caniuse.com/#search=json

@mattab commented on December 25th 2016 Owner

Ok so we won't remove this for a while since we need to support IE7 and IE6. Leaving opened, but decreasing priority

@filipre commented on August 24th 2017

Any updates on this? I guess you are doing this due to contract duties but still encouraging the user to use IE6 sounds extremely insecure.

@mattab commented on September 18th 2017 Owner

I guess you are doing this due to contract duties but still encouraging the user to use IE6 sounds extremely insecure.

we don't encourage users to use IE6, but we need to track users on IE6 and IE7 and cannot drop those users from tracking yet (we may in the future, though)

Powered by GitHub Issue Mirror