Skip to content

Conversation

@leeoniya
Copy link

@leeoniya leeoniya commented Dec 15, 2019

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 -c is 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 yargs with the much lighter yargs-parser if 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-name package 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!

"gulp-streamify": "^1.0.2",
"gulp-terser": "^1.1.6",
"gulp-zip": "^5.0.0",
"jasmine": "^3.3.0",
Copy link
Contributor

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

Copy link
Author

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');
Copy link
Contributor

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

Copy link
Author

@leeoniya leeoniya Dec 15, 2019

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.

@benmccann
Copy link
Contributor

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)

@leeoniya
Copy link
Author

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.

@kurkle
Copy link
Member

kurkle commented Dec 15, 2019

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.
I also think its time to let IE go, its going to be 2020's when v3 is out.
Its IE that is supported for backward compatibility, IMO that does not mean anything new should support it. And projects that need to support it can just use v2 of chart.js

@leeoniya
Copy link
Author

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";
Copy link
Contributor

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?

Copy link
Author

@leeoniya leeoniya Dec 15, 2019

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";
Copy link
Contributor

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?

Copy link
Contributor

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

@benmccann
Copy link
Contributor

I sent #6836 to switch to buble, so I'll close this one now. Thanks for the suggestion!

@benmccann benmccann closed this Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants