-
Notifications
You must be signed in to change notification settings - Fork 12k
baseline setup to build with rollup #6835
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
Conversation
3013ce7 to
ae6de13
Compare
| "gulp-streamify": "^1.0.2", | ||
| "gulp-terser": "^1.1.6", | ||
| "gulp-zip": "^5.0.0", | ||
| "jasmine": "^3.3.0", |
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.
We probably need a lot of this stuff to make the tests pass. I notice that Travis is currently failing
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.
i started from 0 and only added what was necessary to get the build working. i have no interest in getting the rest of your setup working - i trust you guys can handle it from this point if you like what you see here.
|
|
||
| 'use strict'; | ||
|
|
||
| var moment = require('moment'); |
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.
I think we'll move this file to an external repo and make it optional, but until then won't you need this line or else you'll get errors about moment not being defined? you'll also need the optional rollup plugin
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.
rollup & terser will just treat moment as a global and not touch or mangle it and it works just fine. which is the way it works now when you include moment ahead of loading Chart.js in the browser.
|
Most of the gains here come from disabling the tests and the package build and deployment processes. Unfortunately, I don't think it's going to be realistic for us to take most of this change It looks like the one change we could consider is replacing babel with buble, but I'm not familiar with the two to really know if that's a good idea or not. I'd be hesitant to switch away from babel because it's so much more common and contributors are much more likely to be familiar with it (and it's way easier to google) |
|
yes, babel is much more actively maintained and definitely supports more future js, but its codegen is not as optimized, so the builds are larger. 7k is not much here, but it's 50% of uPlot! i just wanted a way to build Chart.js for myself without installing 500mb of node_modules deps and 200k files. figured i'd share the result. |
|
Buble looks really promising to me. I'e noticed babel is adding quite a lot to the size, and forcing us not to use those things that do not transpile efficiently would only be good in every aspect. |
|
it's not just about IE. i've found ES5 to frequently outperform non-transpiled versions (yes, in late 2019), so i wouldn't discount it until you can show performance parity. i think the pace of new js features & syntax sugar outpaces the vendors' ability to implement deep JIT optimizations for them. |
| @@ -1,146 +1,59 @@ | |||
| /* eslint-env es6 */ | |||
| import resolve from '@rollup/plugin-node-resolve'; | |||
| import cjs from "rollup-plugin-cjs-es"; | |||
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.
What's the difference between this and the rollup-plugin-commonjs that was used before?
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.
cjs-es produced much better code at one point. i re-tested it just now and they're mostly equivalent (i still prefer the intermediate names cjs produces better), and commonjs still injects some helpers that don't seem to be necessary, so the minified build ends up ~800b larger.
| import resolve from '@rollup/plugin-node-resolve'; | ||
| import cjs from "rollup-plugin-cjs-es"; | ||
| import buble from '@rollup/plugin-buble'; | ||
| import { string } from "rollup-plugin-string"; |
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.
What's the difference between this and require('./rollup.plugins').stylesheet that was used before?
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.
Hmm. It looks like it no longer extracts the CSS files that were produced in dist/ when running gulp build before
|
I sent #6836 to switch to buble, so I'll close this one now. Thanks for the suggestion! |
i have severe allergies to dependency bloat, so decided to see what it would take to be able to build Chart.js without Babel and a ton of other heavy deps.
rollup -cis all that's needed.the result is pretty good - it cuts the deps down by at least dozens of MB, thousands of files and the output build is both cleaner and 7 KB smaller (min). i have not run any tests, but i tried the uPlot benchmark in leeoniya/uPlot#65 and everything seems to work well (maybe even faster than your regular build).
you might also consider replacing
yargswith the much lighteryargs-parserif all you need it for is to read the CLI params.https://packagephobia.now.sh/result?p=yargs
https://packagephobia.now.sh/result?p=yargs-parser
the
color-namepackage has CRLF line endings, while everything else has LF. it doesn't cause any issues, but my editor warned of inconsistent line endings in the final non-minified build. however, i didn't want to add or write another dependency to make it uniform - though it would be easy to write a 15 LOC inline rollup plugin to do the regex replace.this PR is not meant to be merged, i just wanted for the diffs to be easily readable.
feel free to take this over, or close the PR.
cheers!