Introducing partial dist bundles

[etpinard]

resolves #684

Here's the output:

[rreusser]

Maybe worth an inline comment to note that Core pulls in scatter. If I were looking to bundle my own, I'd start by copy/pasting this file, and I had to track down the inclusion of scatter.

[rreusser]

(To clarify, I had to stop and think whether core pulled it in or whether it was something pulled in indirectly by bar/pie.)

[rreusser]

Was curious how much bar + pie takes up and what happens if assets are zipped. Created index-minimal.js which is just core, but it doesn't really save much. Numbers are:

plotly-basic.min.js: 533K (176K gzipped)
plotly-minimal.min.js (core only): 508K (169K gzipped)

[etpinard]

@rreusser thanks for your investigations.

I was thinking about outputting a STATS.md file in dist/ on npm run build displaying what trace/components are included, the CDN URL and file size (including the gzip-size) of each bundle. This STATS.md could then be re-used in one of our docs pages.

Moreover, do you think we should add dist to our .npmignore -- is it too much to assume that npm users will make their own bundle? Maybe we should only include the main dist/plotly.js bundle on npm?

[rreusser]

I like stats.md. Considered suggesting adding cli flag to the build task (or maybe adding a bundler command to bin/) so that you at least don't actually have to modify the existing build task source to make plotly build your bundle. Maybe it's only a tiny fraction of people though who are interested enough to do this but not interested in forking, modifying etc.

Yeah, I'd think add dist/ to .npmignore. Maybe leave one, but I feel like if you're installing from npm, it's pretty unlikely you're actually writing require('plotly.js/dist/plotly.js') which seems pointless or doing something like <script src="../node_modules/plotly.js/dist/plotly.js"> which seems just weird, fragile and sketchy.

[etpinard]

@rreusser thanks for you input!

I like the idea of adding a bin/ script. Should it be part of this repo or in a separate one like the lodash-cli?

[rreusser]

Shouldn't be too complicated—probably best to go in plotly.js-cli or similar so that you don't have to do a global install—but should look at how other people do it. This was the source of endless headaches with react native. (i.e. "looks like you accidentally installed react-native globally etc etc" even when you most certainly didn't)

[rreusser]

(But again, only if there's actually a reasonable use-case for this. Otherwise just more to maintain.)

[etpinard]

see preview -> https://gist.github.com/etpinard/05c37e9bc8f1f3078eb88f2b016153b4

[rreusser]

As a fail-(slightly-)safe(r), can you include an html comment at the top of the markdown:

'<!--***********************************************************',
'         This file is auto-generated by `npm run stats`.',
'         Please do not edit this file directly.',
'***************************************************************-->',

[etpinard]

@rreusser

I like stats.md.

I chose instead to output all dist/ information in the dist/README.md - see 5bc1167
This will give a better view to users checking it out on GitHub. The stats tasks will be run automatically every time we make a new release.

Yeah, I'd think add dist/ to .npmignore

I chose otherwise. All partial bundles will be included on the npm dist/ folder. I want to make these partial bundles available on the plot.ly embed pages. Having them available on a npm i install is definitely the easier way to do that at the moment. We should start thinking about removing dist/ entirely from npm, but until then the partial bundles will be fetched on npm i.

or maybe adding a bundler command to bin/

Although I'm not entirely convinced, I'd vote for making a separate plotly.js-bundler repo. Adding a bin command as part of this repo would bump browserify and uglify-js among others from dev-dep to dep which sounds a little intrusive to me. I'm thinking that plotly.js-bundler will be a plotly.js dependency that plotly.js would call on npm run build avoiding potential peerDependencies problems. In any case, I'll open up an issue about this after this PR is merged.

[rreusser]

I'd support a comment here noting that scatter is included in core. The first time I read through this, I was wondering if maybe bar or pie were cartesian and pulled in scatter through that (or some other magic). It's not that hard to figure out, but maybe just a nice sign-post if these are the template for creating your own. See: https://github.com/plotly/plotly.js/blob/master/src/plotly.js#L107-L108

[etpinard]

I'd vote 👎 on adding a comment here (in all the other index files) to avoid making them obsolete when we'll strip more things for the ./core.

I would prefer pointing users to the dist/README.md file where coreModules are listed:

[rreusser]

Reviewed. No big surprises. Mixed feelings on bundling. At the end of the day, I feel like maybe just documenting the existing approach might be adequate. Otherwise we have to sort out all the relative path names, which might just not be worthwhile at this point.

[etpinard]

@rreusser

I feel like maybe just documenting the existing approach might be adequate.

If you have any suggestions on expanding the #Modules section in our README please let us know.

[rreusser]

I guess I failed to care abound those details when I did read that part of the README, and failed to remember it when I cared. People showing up and wanting to bundle would probably start there, so I say LGTM.

There's room to refine and refactor, but even pulling all of this into a standalone module would be a minor version bump (added featured, no existing functionality modified?), so I say 💃🏻