Migrate from Browserify to rollup #5904
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
etimberg
reviewed
Dec 12, 2018
rollup.config.js
Outdated
| plugins: [ | ||
| resolve(), | ||
| commonjs(), | ||
| terser({output: {comments: 'some'}}) |
There was a problem hiding this comment.
How many comments does this leave behind?
There was a problem hiding this comment.
Only the copyright header but I changed it for something more explicit:
terser({
output: {
preamble: banner
}
})
simonbrunel
force-pushed
the
chore/rollup
branch
from
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.
simonbrunel
force-pushed
the
chore/rollup
branch
from
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). |
etimberg
approved these changes
Dec 14, 2018
nagix
approved these changes
Dec 14, 2018
benmccann
approved these changes
Dec 14, 2018
This was referenced Mar 4, 2020
This was referenced Mar 9, 2020
This was referenced Apr 14, 2020
exwm
pushed a commit
to exwm/Chart.js
that referenced
this pull request
Apr 30, 2021
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 watchtask bygulp build --watchto be consistent with the otherunittestanddocswatching syntax.npm installgulp buildChart.jsChart.min.jsChart.bunble.jsChart.bundle.min.jsI'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
distoutputs:chartjs-dist-browserify.zip
chartjs-dist-rollup.zip
Fixes #2906
Fixes #5901