Switch from browserify to webpack in order to be able to use most recent ES modules & allow JavaScript syntax beyond ES5
#6355
Conversation
|
@archmoj this is looking great! My biggest comment so far: can we use |
| } | ||
| }, | ||
| plugins: [ | ||
| new NodePolyfillPlugin({ includeAliases: ['process'] }) |
There was a problem hiding this comment.
Did you figure out what this is needed for? Is it something in our code or in a dependency?
webpack.config.js
Outdated
| 'stream': require.resolve('stream-browserify'), | ||
| 'buffer': require.resolve('buffer/'), | ||
| 'assert': require.resolve('assert/') |
There was a problem hiding this comment.
These too - where are these needed? (It's OK if they are needed, I'm mostly just curious, also relevant for anyone who might include plotly.js from source, not dist bundles)
There was a problem hiding this comment.
Good call. assert and buffer are no longer needed. Addressed in 9fc5987.
|
This reverts commit 834d411.
| watch: !argv.nowatch, | ||
| debug: true | ||
| webpack: { | ||
| target: ['web', 'es5'], |
There was a problem hiding this comment.
Have you considered targeting es6? https://betterprogramming.pub/do-we-still-need-to-compile-es6-javascript-code-into-es5-in-2022-f5ec70509e9b
There was a problem hiding this comment.
We still want to continuing support es5 syntax in our output.
But now the input syntax i.e. plotly.js source files as well as node_modules could be in es6 and further.
There was a problem hiding this comment.
@archmoj FWIW we just dropped IE support in Dash - and we did it in a minor https://github.com/plotly/dash/releases/tag/v2.7.0
I'd be comfortable doing the same in plotly.js, but open to discussing.
There was a problem hiding this comment.
I opened #6366.
Let's investigate it & decide on that separately.
- but I keep it during release process
The main part of config is reused now. |
|
Bundle sizes are slightly reduced and are looking good to my eyes.
|
| var makeWatchifiedBundle = require('../../tasks/util/watchified_bundle'); | ||
| var shortcutPaths = require('../../tasks/util/shortcut_paths'); | ||
| var config = require('../../webpack.config.js'); | ||
| config.optimization = { minimize: false }; |
There was a problem hiding this comment.
Ideally even the test build would be minified, to give us further confidence we're testing the behavior of the real bundle, plus a sourcemap. Right now with no sourcemap all I see in the test dashboard is the single huge unminified bundle - all the code is there unmodified but it'd be a pain to connect it back to the source files during debugging.
There was a problem hiding this comment.
The debugging is addressed in 458921f.
Now using development mode, the bundle content is different from production mode; but it builds & rebuilds faster.
This should be good for now. And we could further improve this configuration.
There was a problem hiding this comment.
That'll work - what you see in the debugger isn't quite the same as the source files - it's been reformatted a bit and the require statements are transformed - but it's pretty close and easy to move from one to another 😄
@plotly/plotly_js
TODOs:
regl_codegen/devtools.jssimilar totest_dashboard/devtools.js.glslifytransform for regl based tracesstackgl_modulesusing webpackstrict_d3.js? Possibly no!watchified_bundle.js.scattergltest failures inwebgl-jasminecontainer.LICENSE.txtfilescompress_attributestransform when when creating bundles other thanplotly-with-meta.jsplotly-with-meta.js