-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from Browserify to rollup #5904
Conversation
rollup.config.js
Outdated
plugins: [ | ||
resolve(), | ||
commonjs(), | ||
terser({output: {comments: 'some'}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many comments does this leave behind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the copyright header but I changed it for something more explicit:
terser({
output: {
preamble: banner
}
})
c5a8356
to
f0f6f00
Compare
Browserify isn't optimal bundling Chart.js because it adds too many internal wrappers, doesn't handle external/global dependencies and doesn't provide a way to generate ESM builds. Therefore, it seems the right choice to switch to rollup, so move all the build process in `rollup.config.js` and make Gulp to execute `rollup -c`. We also had to switch to Terser instead of UglifyJS because this last one contains a breaking bug. Note that tests now use the exact same rollup config as our builds (the minified one) to ensure that the bundling and minification steps don't break anything. Finally, replace the `gulp watch` task by `gulp build --watch` to be consistent with the other `unittest` and `docs` watching syntax.
f0f6f00
to
3e43f98
Compare
@etimberg I also changed the copyright header to be more concise: Before: /*!
* Chart.js
* http://chartjs.org/
* Version: 2.7.3
*
* Copyright 2018 Chart.js Contributors
* Released under the MIT license
* https://github.com/chartjs/Chart.js/blob/master/LICENSE.md
*/ After: /*!
* Chart.js v2.7.3
* https://www.chartjs.org
* (c) 2018 Chart.js Contributors
* Released under the MIT License
*/ |
@simonbrunel can you share what the breaking bug is for uglify? |
@benmccann I think it's this one mishoo/UglifyJS#3278 (and many similar per this comment). |
Browserify isn't optimal bundling Chart.js because it adds too many internal wrappers, doesn't handle external/global dependencies and doesn't provide a way to generate ESM builds. Therefore, it seems the right choice to switch to rollup, so move all the build process in `rollup.config.js` and make Gulp to execute `rollup -c`. We also had to switch to Terser instead of UglifyJS because this last one contains a breaking bug. Note that tests now use the exact same rollup config as our builds (the minified one) to ensure that the bundling and minification steps don't break anything. Finally, replace the `gulp watch` task by `gulp build --watch` to be consistent with the other `unittest` and `docs` watching syntax.
Creating this PR to discuss if it's worth switching to rollup now (v2) instead of waiting the next major release. This change optimizes our build sizes (see following table), anticipates the migration of our code to ES modules (#4478), allows us to generate ESM builds (#5179) without maintaining two separate build pipelines and should fixes a few issues (#2906 and #5901).
Browserify isn't optimal bundling Chart.js because it adds too many internal wrappers, doesn't handle external/global dependencies and doesn't provide a way to generate ESM builds. Therefore, it seems the right choice to switch to rollup.
We also have to switch to Terser instead of UglifyJS because this last one contains a breaking bug. Note that tests now use the exact same rollup config as our builds (the minified one) to ensure that the bundling and minification steps don't break anything. Finally, replace the
gulp watch
task bygulp build --watch
to be consistent with the otherunittest
anddocs
watching syntax.npm install
gulp build
Chart.js
Chart.min.js
Chart.bunble.js
Chart.bundle.min.js
I'm not sure why the build process is slower with rollup (maybe because we have to translate from CommonJS modules to ES6). For those who want to check/test both builds, here is the
dist
outputs:chartjs-dist-browserify.zip
chartjs-dist-rollup.zip
Fixes #2906
Fixes #5901