Skip to content

Custom bundle script details #5527

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 24 commits into from
Mar 2, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9fe3255
improve partial bundle script
archmoj Feb 25, 2021
ca4d557
fixup defaults
archmoj Feb 26, 2021
ba00b93
dev-install exorcist v1.0.1
archmoj Mar 1, 2021
cbb431e
externalize the source map file
archmoj Mar 1, 2021
5aa2234
drop sourcemap and unminified options
archmoj Mar 1, 2021
7de7be9
remove keepIndex option and keep index in lib for sourcemap
archmoj Mar 1, 2021
df8005a
fixup including calendars and transforms in extra bundles
archmoj Mar 1, 2021
efa9357
do not expose calendars option for now - unti one could safely drop them
archmoj Mar 1, 2021
9ca5d44
centralize function to create transforms and traces lists
archmoj Mar 1, 2021
9d34652
fixup debug X sourcemap intersect
archmoj Mar 1, 2021
0869970
filter filenames when creating traces and transforms lists
archmoj Mar 1, 2021
7d0145a
make only minified with sourcemap
archmoj Mar 1, 2021
fa35c86
drop unused require
archmoj Mar 1, 2021
65efc42
centralize function to test filenames with lowercase
archmoj Mar 1, 2021
b05beef
pass extra options to uglify for sourcemap
archmoj Mar 1, 2021
fc25200
point to bundle
archmoj Mar 1, 2021
bcb7967
improve config
archmoj Mar 1, 2021
9411d7f
put derequire back
archmoj Mar 1, 2021
1e21747
uninstall exorcist v1.0.1 until there prototype pollution is fixed
archmoj Mar 2, 2021
34c09c3
drop sourcemap option
archmoj Mar 2, 2021
6974914
add unminified option
archmoj Mar 2, 2021
4b0f1da
delete index-custom after bundle
archmoj Mar 2, 2021
90bf969
update comment
archmoj Mar 2, 2021
3693985
add a comma to end of calendars require so that it could be easily dr…
archmoj Mar 2, 2021
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
142 changes: 108 additions & 34 deletions tasks/partial_bundle.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var path = require('path');
var minimist = require('minimist');
var runSeries = require('run-series');
var prependFile = require('prepend-file');

Expand All @@ -7,39 +8,97 @@ var common = require('./util/common');
var _bundle = require('./util/browserify_wrapper');

var header = constants.licenseDist + '\n';
var allTransforms = constants.allTransforms;
var allTraces = constants.allTraces;
var mainIndex = constants.mainIndex;

var argv = process.argv;
function isFalse(a) {
return (
a === 'none' ||
a === 'false'
);
}

function inputBoolean(a, dflt) {
return !a ? dflt : !isFalse(a);
}

function inputArray(a, dflt) {
dflt = dflt.slice();

return (
isFalse(a) ? [] :
!a || a === 'all' ? dflt :
a.split(',')
);
}

if(argv.length > 2) {
if(process.argv.length > 2) {
// command line

var traceList = ['scatter']; // added by default
var name;
for(var i = 2; i < argv.length; i++) {
var a = argv[i];
var args = minimist(process.argv.slice(2), {});

// parse arguments
var out = args.out ? args.out : 'custom';
var traces = inputArray(args.traces, allTraces);
var transforms = inputArray(args.transforms, allTransforms);
var calendars = inputBoolean(args.calendars, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

presumably at some point we will make more of the components registered in core optional. In order to support this in a backward-compatible way, perhaps we can make an option like skipcomponents=calendars, to later expand to allow skipping other components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about this a bit. But I think another option that we want to provide in future is which calendar(s) to add. That can explain why calendars logic here is quite similar to transforms and traces. For the moment it is handled by a boolean; but it could be expanded to a list.
Perhaps to avoid confusion we can move calendars code from ./components/calendars to ./calendars now or once we split it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed on slack, for now let's drop the calendars option - always include calendars in every bundle - until we can ensure that unrelated functionality like connectgaps will not break as in #4883.

At ~81kB minified and rarely used, it would be very nice to make these optional eventually, and you're right to want flexibility even when world calendars are requested to only include the necessary ones, not all of them

var keepindex = inputBoolean(args.keepindex, false);
var sourcemaps = inputBoolean(args.sourcemaps, false);
var unminified = inputBoolean(args.unminified, false);

var i, t;

var traceList = ['scatter']; // added by default
for(i = 0; i < traces.length; i++) {
t = traces[i];
if(
allTraces.indexOf(a) !== -1 && // requested
traceList.indexOf(a) === -1 // not added before
traceList.indexOf(t) === -1 // not added before
) {
traceList.push(a);
if(allTraces.indexOf(t) === -1) {
console.error(t, 'is not a valid trace!', 'Valid traces are:', allTraces);
} else {
traceList.push(t);
}
}
if(a.indexOf('--name=') === 0) name = a.replace('--name=', '');
}
if(!name) name = 'custom';
traceList = traceList.sort();

var transformList = [];
for(i = 0; i < transforms.length; i++) {
t = transforms[i];
if(
transformList.indexOf(t) === -1 // not added before
) {
if(allTransforms.indexOf(t) === -1) {
console.error(t, 'is not a valid transform!', 'Valid transforms are:', allTransforms);
} else {
transformList.push(t);
}
}
}
transformList = transformList.sort();

var opts = {
traceList: traceList,
name: name,
transformList: transformList,
calendars: calendars,
sourcemaps: sourcemaps,

index: path.join(constants.pathToBuild, 'index-' + name + '.js'),
dist: path.join(constants.pathToDist, 'plotly-' + name + '.js'),
distMin: path.join(constants.pathToDist, 'plotly-' + name + '.min.js')
name: out,
index: path.join(constants.pathToLib, 'index-' + out + '.js')
};

if(unminified) {
opts.dist = path.join(constants.pathToDist, 'plotly-' + out + '.js');
} else {
opts.distMin = path.join(constants.pathToDist, 'plotly-' + out + '.min.js');
Copy link
Contributor Author

@archmoj archmoj Mar 2, 2021

Choose a reason for hiding this comment

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

To avoid name conflicts (let's say one uses e.g. basic in output name with different traces), wondering instead of plotly-* shouldn't we use plotly_* here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the other hand folks making custom bundles will likely be used to our partial bundles already, so may be tripped up by the switch from - to _. Let's keep -, these bundles won't be published anyway.

}

if(!keepindex) {
opts.deleteIndex = true;
}

console.log(opts);

var tasks = [];
Expand All @@ -58,40 +117,55 @@ function partialBundle(tasks, opts) {
var dist = opts.dist;
var distMin = opts.distMin;
var traceList = opts.traceList;
var transformList = opts.transformList;
var calendars = opts.calendars;
var sourcemaps = opts.sourcemaps;
var deleteIndex = opts.deleteIndex;

tasks.push(function(done) {
var partialIndex = mainIndex;
allTraces.forEach(function(trace) {
if(traceList.indexOf(trace) === -1) {
var WHITESPACE_BEFORE = '\\s*';
// remove require
var newCode = partialIndex.replace(
new RegExp(
WHITESPACE_BEFORE +
'require\\(\'\\./' + trace + '\'\\),',
'g'), ''
);

// test removal
if(newCode === partialIndex) throw 'Unable to find and drop require for trace: "' + trace + '"';

partialIndex = newCode;

var all = ['calendars'].concat(allTransforms).concat(allTraces);
var includes = (calendars ? ['calendars'] : []).concat(transformList).concat(traceList);
var excludes = all.filter(function(e) { return includes.indexOf(e) === -1; });

excludes.forEach(function(t) {
var WHITESPACE_BEFORE = '\\s*';
// remove require
var newCode = partialIndex.replace(
new RegExp(
WHITESPACE_BEFORE +
'require\\(\'\\./' + t + '\'\\)' +
(t === 'calendars' ? '' : ','), // there is no comma after calendars require
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels a bit fragile - can we either just make the comma optional in the pattern, or add the comma in lib/index so all the lines are equivalent?

'g'), ''
);

// test removal
if(newCode === partialIndex) {
console.error('Unable to find and drop require for ' + t);
throw 'Error generating index for partial bundle!';
}

partialIndex = newCode;
});

common.writeFile(index, partialIndex, done);
});

tasks.push(function(done) {
_bundle(index, dist, {
var bundleOpts = {
debug: sourcemaps,
deleteIndex: deleteIndex,
standalone: 'Plotly',
pathToMinBundle: distMin
}, function() {
};

_bundle(index, dist, bundleOpts, function() {
var headerDist = header.replace('plotly.js', 'plotly.js (' + name + ')');
var headerDistMin = header.replace('plotly.js', 'plotly.js (' + name + ' - minified)');

prependFile(dist, headerDist, common.throwOnError);
prependFile(distMin, headerDistMin, common.throwOnError);
if(dist) prependFile(dist, headerDist, common.throwOnError);
if(distMin) prependFile(distMin, headerDistMin, common.throwOnError);

done();
});
Expand Down
27 changes: 18 additions & 9 deletions tasks/util/browserify_wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,17 @@ module.exports = function _bundle(pathToIndex, pathToBundle, opts, cb) {
}

var b = browserify(pathToIndex, browserifyOpts);
var pending = pathToMinBundle ? 2 : 1;
var pending = (pathToMinBundle && pathToBundle) ? 2 : 1;

function done() {
if(cb && --pending === 0) cb(null);
if(cb && --pending === 0) {
if(opts.deleteIndex) {
console.log('delete', pathToIndex);
fs.unlinkSync(pathToIndex, {});
}

cb(null);
}
}

var bundleStream = b.bundle(function(err) {
Expand All @@ -71,13 +78,15 @@ module.exports = function _bundle(pathToIndex, pathToBundle, opts, cb) {
});
}

bundleStream
.pipe(applyDerequire())
.pipe(fs.createWriteStream(pathToBundle))
.on('finish', function() {
logger(pathToBundle);
done();
});
if(pathToBundle) {
bundleStream
.pipe(applyDerequire())
.pipe(fs.createWriteStream(pathToBundle))
.on('finish', function() {
logger(pathToBundle);
done();
});
}
};

function logger(pathToOutput) {
Expand Down
10 changes: 7 additions & 3 deletions tasks/util/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ var pathToBuild = path.join(pathToRoot, 'build/');

var pathToPlotlyIndex = path.join(pathToLib, 'index.js');
var mainIndex = fs.readFileSync(pathToPlotlyIndex, 'utf-8');
var pathToPlotlyTraces = path.join(pathToSrc, 'traces');
var allTraces = fs.readdirSync(pathToPlotlyTraces);
var allTraces = fs.readdirSync(path.join(pathToSrc, 'traces'));
var allTransforms = fs.readdirSync(path.join(pathToSrc, 'transforms'))
.map(function(e) { return e.replace('.js', ''); });
allTransforms.splice(allTransforms.indexOf('helpers'), 1);

var pathToTopojsonSrc;
try {
Expand Down Expand Up @@ -140,6 +142,8 @@ function makePartialBundleOpts(name) {
return {
name: name,
traceList: partialBundleTraces[name],
transformList: allTransforms,
calendars: true,
index: path.join(pathToLib, 'index-' + name + '.js'),
dist: path.join(pathToDist, 'plotly-' + name + '.js'),
distMin: path.join(pathToDist, 'plotly-' + name + '.min.js')
Expand All @@ -157,10 +161,10 @@ module.exports = {
pathToBuild: pathToBuild,
pathToDist: pathToDist,

allTransforms: allTransforms,
allTraces: allTraces,
mainIndex: mainIndex,
pathToPlotlyIndex: pathToPlotlyIndex,
pathToPlotlyTraces: pathToPlotlyTraces,
pathToPlotlyCore: path.join(pathToSrc, 'core.js'),
pathToPlotlyVersion: path.join(pathToSrc, 'version.js'),
pathToPlotlyBuild: path.join(pathToBuild, 'plotly.js'),
Expand Down