Revise process of making bundles and simplify making partial bundles #5508

archmoj · 2021-02-17T04:05:29Z

Resolves #5484.
Also resolves #4949.

Now one could simply use

npm run partial-bundle pie sunburst --name=custom

to have "browserified" partial bundles (both minified and unminified) in dist folder.
As displayed in the output of this program the temporary index file for such custom build would be created inside build folder not lib and would be labeled as index-custom.js.

Also to mention regarding this PR:

good portion of lib folder is now auto-generated
label traces, transforms and calendars files inside lib folder as "deprecated"
ensure following index.js registering order in all bundles
make separate scripts for extra and custom partial bundles
integrate header_dist task into bundle tasks
remove confusing dev flags from dist bundle

@plotly/plotly_js

- simplify lib folder and generate index files for partial bundles in build - remove confusing dev flags from dist bundle - make a separate script for partial bundles - integrate header_dist into bundle tasks

nicolaskruchten · 2021-02-17T13:54:58Z

I feel like removing all these files from lib will be very disruptive to folks who currently follow the instructions in the main readme about "modules"... Is it really problematic to keep them there? I agree they seem redundant but we've had this readme up there for so long that these files kind of feel like a part of the API at this point...

alexcjohnson · 2021-02-17T15:03:28Z

That's a good point @nicolaskruchten - I suppose we could keep them around but not use them ourselves, and deprecate them so they can be removed in v3?

I guess perhaps the purpose of having this extra layer was to make it clear to folks where the entry points are for Plotly.register. But we never actually said that's what we were doing AFAICT and the pattern isn't that complicated, we can just document it: src/traces/<anything>, src/transforms/<anything>, and src/components/calendars. Would be interesting to see if we can take other stuff from components out of core and into the bundle index files but that's for another time.

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

Q1: In addition to traces, transforms, calendars components do we want to keep index-*.js related to partial bundles in lib?
Q2: Instead of files, can't we simply use soft link to point to traces, transforms, calendars components?

archmoj · 2021-02-17T20:48:25Z

I still think that having those redundant lib files are more confusing than helpful for the users as well as contributors.

BTW here is an example script to generate a custom bundle including pie and sunburst:

npm run partial-bundle pie sunburst name=custom

It also generates and prints the index-custom.js which folks could easily find and use if one ever wants to require.

nicolaskruchten · 2021-02-17T21:03:53Z

I agree they're confusing but I think we should keep them anyway, to reduce the number of breaking changes in 2.0.

archmoj · 2021-02-17T21:22:17Z

I agree they're confusing but I think we should keep them anyway, to reduce the number of breaking changes in 2.0.

If we want to keep this folder around we should cleanup lib/* (similar to what we now do for dist) and generate those files automatically.

nicolaskruchten · 2021-02-17T21:57:54Z

Why don't we just leave lib alone just for historical reasons? it's not really hurting anyone and we can leave a readme in there saying "here for historical reasons" and then change the main readme to point to the new location

archmoj · 2021-02-17T22:17:59Z

Why don't we just leave lib alone just for historical reasons? it's not really hurting anyone and we can leave a readme in there saying "here for historical reasons" and then change the main readme to point to the new location

We shouldn't keep unused files as the API input. Here is one risk:
Everytime one submits a PR we have to check if they changed something in this historical lib.
But if the components listed in lib are created/update/maintained automatically I would be fine with it.

nicolaskruchten · 2021-02-17T23:45:27Z

We can generate them if you feel strongly about it :)

archmoj · 2021-02-19T15:11:15Z

We can generate them if you feel strongly about it :)

100%.
I could argue that we could regenerate those in a patch/minor after v2 if people really needed them.
So this PR is ready to review.

nicolaskruchten · 2021-02-19T17:14:14Z

Ah I think we're misunderstanding each other: I really want these to not be removed in this PR, as this would constitute a new breaking change for people who have followed the instructions in our readme. If you want to generate these identical files moving forward that's fine with me.

archmoj · 2021-02-19T17:28:43Z

Ah I think we're misunderstanding each other: I really want these to not be removed in this PR, as this would constitute a new breaking change for people who have followed the instructions in our readme. If you want to generate these identical files moving forward that's fine with me.

Are you referring to a README other than plotly.js/README.md?

nicolaskruchten · 2021-02-19T17:49:14Z

Nope... the main readme contains this section https://github.com/plotly/plotly.js#modules

archmoj · 2021-02-19T18:01:22Z

Related PR: #202

archmoj · 2021-02-19T18:34:58Z

Also interesting to see here that users were actually looking for a partial-bundle script.

nicolaskruchten · 2021-02-19T18:59:02Z

Yes, I agree that the current readme is not really all that helpful in that it doesn't show how to actually make a custom bundle, which is what this current PR is meant to do. But I also don't want to break things for folks who have followed these instructions that have been in our readme for a long time. Basically: these lib files are part of the documented, public API for 1.x and I don't want this removal to be part of the 2.x breakage please.

- generated component files using preprocess in lib - deprecate component files in lib

… lib folder - generated index files for our partial bundles using extra-bundles script

archmoj · 2021-02-22T16:22:39Z

Yes, I agree that the current readme is not really all that helpful in that it doesn't show how to actually make a custom bundle, which is what this current PR is meant to do. But I also don't want to break things for folks who have followed these instructions that have been in our readme for a long time. Basically: these lib files are part of the documented, public API for 1.x and I don't want this removal to be part of the 2.x breakage please.

Addressed in 01136b7 and aeaf709.

alexcjohnson · 2021-02-22T17:03:19Z

lib/core.js

@@ -1,3 +0,0 @@
-'use strict';
-
-module.exports = require('../src/core');


I guess we'd better bring this file back, as it was referenced in the readme

alexcjohnson · 2021-02-22T17:12:16Z

lib/index-basic.js

 ]);

-module.exports = require('./register_extra')(Plotly);
+module.exports = (function(Plotly) { return Plotly; })(core);


Personally it feels to me like this indirection makes things more confusing than the previous pattern - which with register_extra removed becomes just:

var Plotly = require('../src/core'); Plotly.register([...]); module.exports = Plotly;

Is there another reason for it?

register_extra file is not documented and confusing.
Until we could bump d3 (cc: #5112) or possibly release a @plotly/d3-v3 patch to fix unexpected characters at build time we need a function to the return correct result.

archmoj · 2021-02-22T19:21:39Z

lib/index-cartesian.js

+    require('./violin'),
+    require('./image'),
+    require('./pie'),
+


As you can see, image and pie used to be registered in a different order in cartesian bundles.

archmoj · 2021-02-22T19:22:37Z

lib/index-finance.js

    require('./ohlc'),
    require('./candlestick'),
-    require('./funnel'),
-    require('./waterfall'),
-    require('./indicator')


As you can see, funnel and waterfall and indicator used to be registered in a different order in finance bundles.

nicolaskruchten · 2021-02-22T19:25:58Z

README.md

@@ -96,13 +96,9 @@ Then elsewhere in your code:
 var Plotly = require('./path/to/custom-plotly');
 ```

-#### Non-ascii characters
+Alternatively you could browserify a custom bundle of desired trace modules e.g. `pie` and `choropleth` using
+`npm run partial-bundle pie choropleth name=custom`


it would be nice if this followed the usual command-line flag patterns and hence had --name=custom instead of just name=custom IMO

As a side note I don't want to rely on minimist; but OK I could look for --name when parsing the arguments.

Done in 8d902aa.

alexcjohnson

💃 LGTM!

archmoj added 2 commits February 16, 2021 22:35

rewrite code to generate partial bundles

4bbe89a

- simplify lib folder and generate index files for partial bundles in build - remove confusing dev flags from dist bundle - make a separate script for partial bundles - integrate header_dist into bundle tasks

wrap Plotly to avoid unexpected character

c3a8b48

archmoj added type: maintenance labels Feb 17, 2021

archmoj added this to the NEXT milestone Feb 17, 2021

archmoj added 3 commits February 17, 2021 14:47

add extra_bundles - handle options in partial_bundle

4f862ca

update README on how to use the script to make custom bundles

e4c2f86

remove note concerning non-ascii characters from README

836248c

archmoj added feature something new and removed type: maintenance labels Feb 17, 2021

archmoj requested a review from alexcjohnson February 18, 2021 21:24

archmoj added 2 commits February 22, 2021 11:12

write component files to lib

01136b7

- generated component files using preprocess in lib - deprecate component files in lib

modify util/constants to write index files for our partial bundles in…

aeaf709

… lib folder - generated index files for our partial bundles using extra-bundles script

archmoj requested review from alexcjohnson and removed request for alexcjohnson February 22, 2021 16:22

archmoj changed the title ~~Revise process of making bundles~~ Revise process of making bundles and simplify making partial bundles Feb 22, 2021

alexcjohnson reviewed Feb 22, 2021

View reviewed changes

archmoj added 5 commits February 22, 2021 12:54

do not deprecate components in lib in v2

c937a6f

commit changes to lib when bumping version

d29446e

keep core reference in lib

df67243

reference lib in bundle tests where possible

4772f90

update README

e1c4cc3

archmoj commented Feb 22, 2021

View reviewed changes

nicolaskruchten reviewed Feb 22, 2021

View reviewed changes

archmoj added 2 commits February 22, 2021 14:35

use --name in partial bundle script

8d902aa

add comment to the end of lib/index files

0d00c52

archmoj mentioned this pull request Feb 22, 2021

Partial bundle for the treemap #4949

Closed

simple Plotly return from index

a66d56a

alexcjohnson approved these changes Feb 22, 2021

View reviewed changes

archmoj merged commit d0ddf03 into master Feb 22, 2021

archmoj deleted the improve-mk-bundles branch February 22, 2021 23:55

archmoj mentioned this pull request Apr 22, 2021

Fix the list of traces for publishing packages to npm #5605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise process of making bundles and simplify making partial bundles #5508

Revise process of making bundles and simplify making partial bundles #5508

archmoj commented Feb 17, 2021 •

edited

Loading

nicolaskruchten commented Feb 17, 2021

alexcjohnson commented Feb 17, 2021

archmoj commented Feb 17, 2021

archmoj commented Feb 17, 2021

nicolaskruchten commented Feb 17, 2021

archmoj commented Feb 17, 2021

nicolaskruchten commented Feb 17, 2021

archmoj commented Feb 17, 2021

nicolaskruchten commented Feb 17, 2021

archmoj commented Feb 19, 2021

nicolaskruchten commented Feb 19, 2021

archmoj commented Feb 19, 2021

nicolaskruchten commented Feb 19, 2021

archmoj commented Feb 19, 2021

archmoj commented Feb 19, 2021

nicolaskruchten commented Feb 19, 2021

archmoj commented Feb 22, 2021

alexcjohnson Feb 22, 2021

alexcjohnson Feb 22, 2021

archmoj Feb 22, 2021

archmoj Feb 22, 2021 •

edited

Loading

archmoj Feb 22, 2021

nicolaskruchten Feb 22, 2021

archmoj Feb 22, 2021

archmoj Feb 22, 2021

alexcjohnson left a comment

		@@ -1,3 +0,0 @@
		'use strict';

		module.exports = require('../src/core');

Revise process of making bundles and simplify making partial bundles #5508

Revise process of making bundles and simplify making partial bundles #5508

Conversation

archmoj commented Feb 17, 2021 • edited Loading

nicolaskruchten commented Feb 17, 2021

alexcjohnson commented Feb 17, 2021

archmoj commented Feb 17, 2021

archmoj commented Feb 17, 2021

nicolaskruchten commented Feb 17, 2021

archmoj commented Feb 17, 2021

nicolaskruchten commented Feb 17, 2021

archmoj commented Feb 17, 2021

nicolaskruchten commented Feb 17, 2021

archmoj commented Feb 19, 2021

nicolaskruchten commented Feb 19, 2021

archmoj commented Feb 19, 2021

nicolaskruchten commented Feb 19, 2021

archmoj commented Feb 19, 2021

archmoj commented Feb 19, 2021

nicolaskruchten commented Feb 19, 2021

archmoj commented Feb 22, 2021

alexcjohnson Feb 22, 2021

Choose a reason for hiding this comment

alexcjohnson Feb 22, 2021

Choose a reason for hiding this comment

archmoj Feb 22, 2021

Choose a reason for hiding this comment

archmoj Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

archmoj Feb 22, 2021

Choose a reason for hiding this comment

nicolaskruchten Feb 22, 2021

Choose a reason for hiding this comment

archmoj Feb 22, 2021

Choose a reason for hiding this comment

archmoj Feb 22, 2021

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment

archmoj commented Feb 17, 2021 •

edited

Loading

archmoj Feb 22, 2021 •

edited

Loading