Custom bundle script details #5527

archmoj · 2021-02-25T22:45:36Z

PR to resolve #5521.

Here is how the syntax looks like.

npm run partial-bundle -- \
  --traces sunburst,treemap \
  --transforms filter,sort \
  --out customBundleName

Please note that scatter trace would always be added as it is located in lib/core.js not in lib/index.js.

Also one could use --unminified option to create unminified bundles.

@plotly/plotly_js

nicolaskruchten · 2021-02-26T13:59:50Z

I wonder if a better default wouldn't be minified+sourcemaps, and then a single flag to do unminified+no_sourcemaps... Or maybe we always output all three?

- include all traces, transforms and calendars by default - minify by default

archmoj · 2021-02-26T15:24:17Z

I wonder if a better default wouldn't be minified+sourcemaps, and then a single flag to do unminified+no_sourcemaps... Or maybe we always output all three?

In ca4d557 the default is changed to minified.
But enabling sourcemaps by default can significantly increase the size (3X for scatter only bundle); so I still think it should not be the default. Also to mention having sourcemaps false and --nosurcemaps does not look great.

nicolaskruchten · 2021-02-26T15:30:40Z

sourcemaps should not increase the bundle size... sourcemaps should appear in a separate file next to the bundle, right?

archmoj · 2021-02-26T15:39:26Z

sourcemaps should not increase the bundle size... sourcemaps should appear in a separate file next to the bundle, right?

Right now it is included in the bundle.

archmoj · 2021-02-26T15:40:45Z

sourcemaps should not increase the bundle size... sourcemaps should appear in a separate file next to the bundle, right?

Right now it is included in the bundle.

Maybe we should try this: browserify/browserify#339

nicolaskruchten · 2021-02-26T15:46:17Z

Right now it is included in the bundle.

Let's definitely not do that :) Ideally sourcemaps are just another file on the side, that is only loaded by dev tools, and otherwise ignored by regular users.

archmoj · 2021-02-26T17:01:20Z

Right now it is included in the bundle.

Let's definitely not do that :) Ideally sourcemaps are just another file on the side, that is only loaded by dev tools, and otherwise ignored by regular users.

It appears achieving that with browserify can complicate our build process even more.

nicolaskruchten · 2021-02-26T17:04:58Z

Hmmm that's quite disappointing. Are you sure? this is a very standard way of using sourcemaps... If they must be inlined and therefore bloat the minified bundle, there'd be no point in minifying :)

alexcjohnson · 2021-02-27T13:34:08Z

I don't know if this is what you mean by "complicate our build process even more" - at least on the command line it seems straightforward

https://github.com/browserify/browserify-handbook#exorcist

alexcjohnson · 2021-02-27T13:47:42Z

tasks/partial_bundle.js

+    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);


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?

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?

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

tasks/partial_bundle.js

alexcjohnson · 2021-02-27T14:01:13Z

If we can get the source map in a separate file, then yes minified with source map should be the default. At that point I don't see a reason you'd want to invert either of those options (for dash and dash components that's all we provide), but I also don't see much harm keeping them.

archmoj · 2021-03-01T14:38:17Z

I don't know if this is what you mean by "complicate our build process even more" - at least on the command line it seems straightforward

https://github.com/browserify/browserify-handbook#exorcist

Nice find. I was looking at other npm modules actually. BTW exorcist uses an old version of minimist which should be patched to avoid npm audit security warning. So I opened an issue here.

tasks/util/constants.js

Co-authored-by: Alex Johnson <[email protected]>

alexcjohnson · 2021-03-01T18:16:30Z

tasks/util/browserify_wrapper.js

+    if(sourceMap) {
+        bundleStream
+            .pipe(applyDerequire())
+            .pipe(exorcist(sourceMap))


This is generating the sourcemap of the unminified bundle. The real value is in a sourcemap of the minified bundle (which, for clarity, gets extension .min.js.map), then you never need the unminified bundle.

Good call. Addressed in 7d0145a.

archmoj · 2021-03-01T23:22:54Z

FYI - thlorenz/exorcist#47

archmoj · 2021-03-02T17:13:37Z

tasks/partial_bundle.js

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


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?

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.

alexcjohnson · 2021-03-02T17:45:20Z

tasks/partial_bundle.js

+                new RegExp(
+                    WHITESPACE_BEFORE +
+                    'require\\(\'\\./' + t + '\'\\)' +
+                    (t === 'calendars' ? '' : ','), // there is no comma after calendars require


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?

alexcjohnson

💃 LGTM! Just one nonblocking comment remaining re: the regexp comma.

…opped later

improve partial bundle script

9fe3255

archmoj added feature something new status: reviewable labels Feb 25, 2021

archmoj added this to the NEXT milestone Feb 25, 2021

fixup defaults

ca4d557

- include all traces, transforms and calendars by default - minify by default

alexcjohnson reviewed Feb 27, 2021

View reviewed changes

tasks/partial_bundle.js Outdated Show resolved Hide resolved

archmoj added 3 commits March 1, 2021 10:48

dev-install exorcist v1.0.1

ba00b93

externalize the source map file

cbb431e

drop sourcemap and unminified options

5aa2234

alexcjohnson reviewed Mar 1, 2021

View reviewed changes

tasks/util/constants.js Outdated Show resolved Hide resolved

archmoj and others added 6 commits March 1, 2021 11:55

remove keepIndex option and keep index in lib for sourcemap

7de7be9

fixup including calendars and transforms in extra bundles

df8005a

do not expose calendars option for now - unti one could safely drop them

efa9357

centralize function to create transforms and traces lists

9ca5d44

fixup debug X sourcemap intersect

9d34652

filter filenames when creating traces and transforms lists

0869970

Co-authored-by: Alex Johnson <[email protected]>

archmoj requested a review from alexcjohnson March 1, 2021 17:52

alexcjohnson reviewed Mar 1, 2021

View reviewed changes

archmoj added 7 commits March 1, 2021 13:47

make only minified with sourcemap

7d0145a

drop unused require

fa35c86

centralize function to test filenames with lowercase

65efc42

pass extra options to uglify for sourcemap

b05beef

point to bundle

fc25200

improve config

bcb7967

put derequire back

9411d7f

archmoj added 5 commits March 2, 2021 09:29

uninstall exorcist v1.0.1 until there prototype pollution is fixed

1e21747

drop sourcemap option

34c09c3

add unminified option

6974914

delete index-custom after bundle

4b0f1da

update comment

90bf969

archmoj commented Mar 2, 2021

View reviewed changes

alexcjohnson reviewed Mar 2, 2021

View reviewed changes

alexcjohnson approved these changes Mar 2, 2021

View reviewed changes

add a comma to end of calendars require so that it could be easily dr…

3693985

…opped later

archmoj merged commit ed52205 into master Mar 2, 2021

archmoj deleted the partial-bundle-API branch March 2, 2021 18:43

archmoj mentioned this pull request Mar 8, 2021

Revise scatter register to allow custom partial bundles without scatter included by default #5535

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom bundle script details #5527

Custom bundle script details #5527

archmoj commented Feb 25, 2021 •

edited

Loading

nicolaskruchten commented Feb 26, 2021

archmoj commented Feb 26, 2021

nicolaskruchten commented Feb 26, 2021

archmoj commented Feb 26, 2021

archmoj commented Feb 26, 2021

nicolaskruchten commented Feb 26, 2021

archmoj commented Feb 26, 2021

nicolaskruchten commented Feb 26, 2021

alexcjohnson commented Feb 27, 2021

alexcjohnson Feb 27, 2021

archmoj Mar 1, 2021

alexcjohnson Mar 1, 2021

alexcjohnson commented Feb 27, 2021

archmoj commented Mar 1, 2021

alexcjohnson Mar 1, 2021

archmoj Mar 1, 2021

archmoj commented Mar 1, 2021

archmoj Mar 2, 2021 •

edited

Loading

alexcjohnson Mar 2, 2021

alexcjohnson Mar 2, 2021

alexcjohnson left a comment

Custom bundle script details #5527

Custom bundle script details #5527

Conversation

archmoj commented Feb 25, 2021 • edited Loading

nicolaskruchten commented Feb 26, 2021

archmoj commented Feb 26, 2021

nicolaskruchten commented Feb 26, 2021

archmoj commented Feb 26, 2021

archmoj commented Feb 26, 2021

nicolaskruchten commented Feb 26, 2021

archmoj commented Feb 26, 2021

nicolaskruchten commented Feb 26, 2021

alexcjohnson commented Feb 27, 2021

alexcjohnson Feb 27, 2021

Choose a reason for hiding this comment

archmoj Mar 1, 2021

Choose a reason for hiding this comment

alexcjohnson Mar 1, 2021

Choose a reason for hiding this comment

alexcjohnson commented Feb 27, 2021

archmoj commented Mar 1, 2021

alexcjohnson Mar 1, 2021

Choose a reason for hiding this comment

archmoj Mar 1, 2021

Choose a reason for hiding this comment

archmoj commented Mar 1, 2021

archmoj Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

alexcjohnson Mar 2, 2021

Choose a reason for hiding this comment

alexcjohnson Mar 2, 2021

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment

archmoj commented Feb 25, 2021 •

edited

Loading

archmoj Mar 2, 2021 •

edited

Loading