Skip to content

Commit

Permalink
Better usage of JSON-js polyfills
Browse files Browse the repository at this point in the history
A couple things were weird with how JSON is polyfilled in the library.

First, we have a flag
__USE_JSON__ which is set at build time to output two different bundles, .js and .nojson.js which
leads to a bit of confusion from the end user as to which to use.

Second, this is only done because
JSON-js/json2.js uses eval in it's parse function which violates CSP, but there is an alternate
parse implementation that uses a state machine and no eval which obviates the need for this .js and
.nojson.js split.

Third, the way this was used I think was buggy to begin with. The custom json2.js parse and
stringify functions should only be used if JSON does not already exist natively with this
functionality. It is already built into json2.js to check for this and only polyfill if needed. To
get that to work, you just need to pass in the global JSON and let it do it's work. The problem is
then when we decide to use the polyfill we pass in an empty object always which forces those
polyfills to always be used. It does not seem like that is intended behaviour.

We are not affecting the global JSON object so this polyfill will not be visible outside our
library, we only polyfill the functions that are not already present on the possibly existing JSON
global, and we use a version of parse that does not use eval and therefore is CSP safe.
  • Loading branch information
rokob committed Feb 22, 2017
1 parent a17c6ac commit 754069a
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 28 deletions.
26 changes: 12 additions & 14 deletions src/bundles/rollbar.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
/* globals __USE_JSON__ */


var globalnotifier = require('../globalnotifier');
var notifier = require('../notifier');


function setupJSON() {
var JSONObject = typeof JSON === 'undefined' ? {} : JSON;

if (__USE_JSON__) {
// This adds the script to this context. We need it since this library
// is not a CommonJs or AMD module.
var setupCustomJSON = require('../../vendor/JSON-js/json2.js');

var customJSON = {};
setupCustomJSON(customJSON);

JSONObject = customJSON;
var JSONObject = {};

if (typeof JSON !== 'undefined') {
if (typeof JSON.stringify === 'function') {
JSONObject.stringify = JSON.stringify;
}
if (typeof JSON.parse === 'function') {
JSONObject.parse = JSON.parse;
}
}

var setupCustomJSON = require('../../vendor/JSON-js/json2.js');
setupCustomJSON(JSONObject);

globalnotifier.setupJSON(JSONObject);
}

Expand Down
16 changes: 2 additions & 14 deletions webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,6 @@ var defaults = require('./defaults');

var outputPath = './dist/';

var jsonDefines = {
__USE_JSON__: true
};

var noJsonDefines = {
__USE_JSON__: false
};

var defaultsPlugin = new webpack.DefinePlugin(defaults);
var uglifyPlugin = new webpack.optimize.UglifyJsPlugin({
// We've had some reports of the sourceMappingURL comment causing problems in Firefox.
Expand All @@ -21,8 +13,6 @@ var uglifyPlugin = new webpack.optimize.UglifyJsPlugin({
sourceMap: false,
minimize: true
});
var useJsonPlugin = new webpack.DefinePlugin(jsonDefines);
var notUseJsonPlugin = new webpack.DefinePlugin(noJsonDefines);

var snippetConfig = {
name: 'snippet',
Expand Down Expand Up @@ -203,9 +193,7 @@ function generateBuildConfig(name, plugins) {
addNamedAMDToConfig(config, name, plugins);
}

generateBuildConfig('[name].js', [useJsonPlugin]);
generateBuildConfig('[name].min.js', [useJsonPlugin, uglifyPlugin]);
generateBuildConfig('[name].nojson.js', [notUseJsonPlugin]);
generateBuildConfig('[name].nojson.min.js', [notUseJsonPlugin, uglifyPlugin]);
generateBuildConfig('[name].js', []);
generateBuildConfig('[name].min.js', [uglifyPlugin]);

module.exports = config;

0 comments on commit 754069a

Please sign in to comment.