Enable compress attributes browserify transform by default #1584

etpinard · 2017-04-11T21:05:50Z

as discussed in rreusser/plotly-mock-viewer#5 (comment), we should really be compressing our attribute declarations in all browserify builds by default - to make sure that customs bundles don't include (useless) attribute description strings.

Note that, using the ignoreTransform browserify options with can still produce dist/plotly-with-meta.js as before.

TODO:

In this PR, I'm referring to the compress_attributes.js transform using a relative (UNIX) path. We should make sure that this works also on Windows machines before merging. If that doesn't work we can always publish compress_attributes.js on npm and refer to it as a dependency in node_modules/ like we currently do for glslify.
move the plotschema_test.js suite to bundle_tests/ and make it use plotly-with-meta.js

cc @rreusser

- so that it is enabled by default on browserify builds - mv it to `./tasks/` for better discoveribility - ignore it when bundling plotly-with-meta.js bundle - 🔪 other refs to compress_attributes.js

rreusser · 2017-09-21T07:02:31Z

Apologies I missed in the first place, but I wholeheartedly support this. The only exception would be if it makes it difficult to compile with metadata. So my question is: how do you compile with metadata when this is present?

rreusser · 2017-09-21T07:03:19Z

Oh, just saw linked comment:

--ignore-transform=MODULE

rreusser · 2017-09-21T07:06:48Z

tasks/util/browserify_wrapper.js

-    browserifyOpts.transform = outputMinified ? [compressAttributes] : [];
+
+    if(opts.noCompress) {
+        browserifyOpts.ignoreTransform = './tasks/compress_attributes.js';


👏 This seems better by default, and tasks/ seems like a better location than tasks/util

rreusser · 2017-09-21T07:07:28Z

tasks/util/watchified_bundle.js

@@ -6,7 +6,6 @@ var prettySize = require('prettysize');

 var constants = require('./constants');
 var common = require('./common');
-var compressAttributes = require('./compress_attributes');


Nice that this just disappears. Opt-opt also scopes the transform better so it doesn't need to be applied globally 👍

rreusser · 2017-09-21T07:07:42Z

tasks/bundle.js

@@ -41,7 +41,8 @@ _bundle(constants.pathToPlotlyGeoAssetsSrc, constants.pathToPlotlyGeoAssetsDist,
 // Browserify the plotly.js with meta
 _bundle(constants.pathToPlotlyIndex, constants.pathToPlotlyDistWithMeta, {
    standalone: 'Plotly',
-    debug: DEV
+    debug: DEV,
+    noCompress: true


rreusser · 2017-09-21T07:09:00Z

Apologies I didn't leave more meaningful feedback the first time around. This definitely seems like a good idea. It's really the exception rather than the norm that you'd even want the extra metadata. It looks like it needs a quick refresh to resolve a conflict or two, but assuming that's just trivial merging, it gets the 💃 from me.

Edit: and pending windows check.

etpinard · 2017-11-17T16:27:13Z

Can anyone with a window laptop try running npm run build off this branch? I'm a little worried about this line which uses a UNIX-style path.

Maybe @dfcreative could help.

... where we ignore the compress_attributes.js transform on bundling.

etpinard · 2018-09-10T16:30:07Z

Ping @antoinerg (who has access to a Windows "dev" 💻 )

Could you pull down this branch and run npm run build to check that the added UNIX-style path to a browserify transform in our package.json works ok on Windows?

To check that things work:

npm run build, then
grep "description:" dist/plotly.js (or some windows equivalent) should output:

    return Lib.extendFlat({}, attributes, { description: description });
 *     extra.description: extra description. N.B we use
 *     opts.description: where & how this font is used

i.e. only three occurrences of description:

and grep "description:" dist/plotly-with-meta.js | wc -l should be 973 (i.e. 973 lines with description: in it)

Thanks!

antoinerg · 2018-09-10T18:20:48Z

@etpinard: I confirm that things work on my Windows 10 machine 🎉

etpinard · 2018-09-10T18:36:08Z

Fantastic!

Attribute meta will be stripped out of bundles (made with bundlers that support browserify transforms) by default starting in v1.41.0!

... post plotly/plotly.js#1584

etpinard added 2 commits April 11, 2017 16:54

bump browserify version

74f9f39

add compress_attributes.js transform to "browserify" field

3485c0e

- so that it is enabled by default on browserify builds - mv it to `./tasks/` for better discoveribility - ignore it when bundling plotly-with-meta.js bundle - 🔪 other refs to compress_attributes.js

etpinard added status: in progress labels Apr 11, 2017

rreusser reviewed Sep 21, 2017

View reviewed changes

etpinard added 3 commits September 10, 2018 11:35

Merge branch 'master' into compress-attr-transform-by-default

03be8ff

🔪 now obsolete compressAttrs arg

ce2a33a

move plotschema_test to bundle_tests/

7e7baaf

... where we ignore the compress_attributes.js transform on bundling.

etpinard added status: reviewable and removed status: in progress labels Sep 10, 2018

etpinard added this to the v1.41.0 milestone Sep 10, 2018

rm info about compress_attributes transform

9af5948

etpinard merged commit 1b6c9a3 into master Sep 10, 2018

etpinard deleted the compress-attr-transform-by-default branch September 10, 2018 18:59

etpinard mentioned this pull request Sep 14, 2018

Should we expose a high-level bundler module / CLI ? #768

Closed

etpinard added a commit to rreusser/plotly-mock-viewer that referenced this pull request Oct 9, 2018

update keepMeta logic

03301c1

... post plotly/plotly.js#1584

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Enable compress attributes browserify transform by default #1584

Enable compress attributes browserify transform by default #1584

etpinard commented Apr 11, 2017 •

edited

Loading

rreusser commented Sep 21, 2017

rreusser commented Sep 21, 2017

rreusser Sep 21, 2017

rreusser Sep 21, 2017

rreusser Sep 21, 2017

rreusser commented Sep 21, 2017 •

edited

Loading

etpinard commented Nov 17, 2017

etpinard commented Sep 10, 2018

antoinerg commented Sep 10, 2018 •

edited

Loading

etpinard commented Sep 10, 2018

Enable compress attributes browserify transform by default #1584

Enable compress attributes browserify transform by default #1584

Conversation

etpinard commented Apr 11, 2017 • edited Loading

rreusser commented Sep 21, 2017

rreusser commented Sep 21, 2017

rreusser Sep 21, 2017

Choose a reason for hiding this comment

rreusser Sep 21, 2017

Choose a reason for hiding this comment

rreusser Sep 21, 2017

Choose a reason for hiding this comment

rreusser commented Sep 21, 2017 • edited Loading

etpinard commented Nov 17, 2017

etpinard commented Sep 10, 2018

antoinerg commented Sep 10, 2018 • edited Loading

etpinard commented Sep 10, 2018

etpinard commented Apr 11, 2017 •

edited

Loading

rreusser commented Sep 21, 2017 •

edited

Loading

antoinerg commented Sep 10, 2018 •

edited

Loading