Skip to content

Fix minified dist bundles #1094

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

Merged
merged 5 commits into from
Oct 27, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 2 additions & 51 deletions tasks/bundle.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,6 @@
var fs = require('fs');
var path = require('path');

var browserify = require('browserify');
var UglifyJS = require('uglify-js');

var constants = require('./util/constants');
var common = require('./util/common');
var compressAttributes = require('./util/compress_attributes');
var patchMinified = require('./util/patch_minified');
var doesFileExist = common.doesFileExist;
var _bundle = require('./util/browserify_wrapper');

/*
* This script takes one argument
Expand All @@ -26,6 +18,7 @@ var DEV = (arg === 'dev') || (arg === '--dev');


// Check if style and font build files are there
var doesFileExist = common.doesFileExist;
if(!doesFileExist(constants.pathToCSSBuild) || !doesFileExist(constants.pathToFontSVG)) {
throw new Error([
'build/ is missing one or more files',
Expand Down Expand Up @@ -59,45 +52,3 @@ constants.partialBundlePaths.forEach(function(pathObj) {
pathToMinBundle: pathObj.distMin
});
});

function _bundle(pathToIndex, pathToBundle, opts) {
opts = opts || {};

// do we output a minified file?
var pathToMinBundle = opts.pathToMinBundle,
outputMinified = !!pathToMinBundle && !opts.debug;

var browserifyOpts = {};
browserifyOpts.standalone = opts.standalone;
browserifyOpts.debug = opts.debug;
browserifyOpts.transform = outputMinified ? [compressAttributes] : [];

var b = browserify(pathToIndex, browserifyOpts),
bundleWriteStream = fs.createWriteStream(pathToBundle);

bundleWriteStream.on('finish', function() {
logger(pathToBundle);
});

b.bundle(function(err, buf) {
if(err) throw err;

if(outputMinified) {
var minifiedCode = UglifyJS.minify(buf.toString(), constants.uglifyOptions).code;
minifiedCode = patchMinified(minifiedCode);

fs.writeFile(pathToMinBundle, minifiedCode, function(err) {
if(err) throw err;

logger(pathToMinBundle);
});
}
})
.pipe(bundleWriteStream);
}

function logger(pathToOutput) {
var log = 'ok ' + path.basename(pathToOutput);

console.log(log);
}
31 changes: 11 additions & 20 deletions tasks/cibundle.js
Original file line number Diff line number Diff line change
@@ -1,33 +1,24 @@
var fs = require('fs');

var browserify = require('browserify');

var constants = require('./util/constants');
var common = require('./util/common');
var compressAttributes = require('./util/compress_attributes');
var _bundle = require('./util/browserify_wrapper');

/*
* Trimmed down version of ./bundle.js for CI testing
*
* Outputs plotly.js bundle in build/ and
* plotly-geo-assets.js bundle in dist/
* in accordance with test/image/index.html
* Outputs:
*
* - plotly.js bundle in build/
* - plotly-geo-assets.js bundle in dist/ (in accordance with test/image/index.html)
* - plotly.min.js bundle in dist/ (for requirejs test)
*/


// Browserify plotly.js
browserify(constants.pathToPlotlyIndex, {
// Browserify plotly.js and plotly.min.js
_bundle(constants.pathToPlotlyIndex, constants.pathToPlotlyBuild, {
standalone: 'Plotly',
transform: [compressAttributes]
})
.bundle(common.throwOnError)
.pipe(fs.createWriteStream(constants.pathToPlotlyBuild));

pathToMinBundle: constants.pathToPlotlyDistMin,
});

// Browserify the geo assets
browserify(constants.pathToPlotlyGeoAssetsSrc, {
_bundle(constants.pathToPlotlyGeoAssetsSrc, constants.pathToPlotlyGeoAssetsDist, {
standalone: 'PlotlyGeoAssets'
})
.bundle(common.throwOnError)
.pipe(fs.createWriteStream(constants.pathToPlotlyGeoAssetsDist));
});
71 changes: 71 additions & 0 deletions tasks/util/browserify_wrapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
var fs = require('fs');
var path = require('path');

var browserify = require('browserify');
var UglifyJS = require('uglify-js');

var constants = require('./constants');
var compressAttributes = require('./compress_attributes');
var patchMinified = require('./patch_minified');

/** Convenience browserify wrapper
*
* @param {string} pathToIndex path to index file to bundle
* @param {string} pathToBunlde path to destination bundle
*
* @param {object} opts
*
* Browserify options:
* - standalone {string}
* - debug {boolean} [optional]
*
* Additional option:
* - pathToMinBundle {string} path to destination minified bundle
*
* Outputs one bundle (un-minified) file if opts.pathToMinBundle is omitted
* or opts.debug is true. Otherwise outputs two file: one un-minified bundle and
* one minified bundle.
*
* Logs basename of bundle when completed.
*/
module.exports = function _bundle(pathToIndex, pathToBundle, opts) {
opts = opts || {};

// do we output a minified file?
var pathToMinBundle = opts.pathToMinBundle,
outputMinified = !!pathToMinBundle && !opts.debug;

var browserifyOpts = {};
browserifyOpts.standalone = opts.standalone;
browserifyOpts.debug = opts.debug;
browserifyOpts.transform = outputMinified ? [compressAttributes] : [];

var b = browserify(pathToIndex, browserifyOpts),
bundleWriteStream = fs.createWriteStream(pathToBundle);

bundleWriteStream.on('finish', function() {
logger(pathToBundle);
});

b.bundle(function(err, buf) {
if(err) throw err;

if(outputMinified) {
var minifiedCode = UglifyJS.minify(buf.toString(), constants.uglifyOptions).code;
minifiedCode = patchMinified(minifiedCode);

fs.writeFile(pathToMinBundle, minifiedCode, function(err) {
if(err) throw err;

logger(pathToMinBundle);
});
}
})
.pipe(bundleWriteStream);
};

function logger(pathToOutput) {
var log = 'ok ' + path.basename(pathToOutput);

console.log(log);
}
31 changes: 27 additions & 4 deletions tasks/util/patch_minified.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,38 @@
var STR_TO_REPLACE = 'require("+a(r)+");';
var STR_NEW = 'require("+ a(r) +");';
var ALPHABET = 'abcdefghijklmnopqrstuvwxyz'.split('');
var FRONT = 'require("+';
var BACK = '+");';

/* Uber hacky in-house fix to
*
* https://github.com/substack/webworkify/issues/29
*
* so that plotly.min.js loads in Jupyter NBs, more info here:
*
* https://github.com/plotly/plotly.py/pull/545
* - https://github.com/plotly/plotly.py/pull/545
* - https://github.com/plotly/plotly.js/pull/914
* - https://github.com/plotly/plotly.js/pull/1094
*
* For example, this routine replaces
* 'require("+o(s)+")' -> 'require("+ o(s) +")'
*
* But works for any 1-letter variable that uglify-js may output.
*
*/
module.exports = function patchMinified(minifiedCode) {
return minifiedCode.replace(STR_TO_REPLACE, STR_NEW);
for(var i = 0; i < ALPHABET.length; i++) {
var li = ALPHABET[i];

for(var j = 0; j < ALPHABET.length; j++) {
var lj = ALPHABET[j];

var MIDDLE = li + '(' + lj + ')';

var strOld = FRONT + MIDDLE + BACK,
strNew = FRONT + ' ' + MIDDLE + ' ' + BACK;

minifiedCode = minifiedCode.replace(strOld, strNew);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's a way to do using Regex, please let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rreusser 's got the answer

minifiedCod.replace(/require\("\+(\w)\((\w)\)\+"\)/, 'require("+ $1($2) +")')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could also be done using .split(strOld).join(strNew) but I have no idea if it would be any faster in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@rreusser rreusser Oct 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A slight modification for lower or uppercase:

/require\("\+([A-z])\(([A-z])\)\+"\)/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I counter with https://regex101.com/

}
}

return minifiedCode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hilarious. Do we need to check for uppercase letters or does it stick to all lowers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Does uglify-js use uppercase lettes? I don't know. But anyway, @rreusser's solution should cover that case too.

Copy link
Contributor

@rreusser rreusser Oct 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always forget the semantics of A-z vs. \W vs. \w. Okay:

  • \w: Matches any alphanumeric character including the underscore. Equivalent to [A-Za-z0-9_].
  • \W: Matches any non-word character. Equivalent to [^A-Za-z0-9_]. (= negation)
  • [A-Za-z] any lower or uppercase character
  • [A-z] maybe the same?

};