Chart.js icon indicating copy to clipboard operation
Chart.js copied to clipboard

fix: treeshaking

Open dangreen opened this issue 1 year ago • 11 comments

https://github.com/chartjs/Chart.js/issues/10163 fix

Before (build of each export):

378 kB   Chart.js
378 kB   BarController.js
378 kB   BubbleController.js
378 kB   DoughnutController.js
378 kB   LineController.js
378 kB   PolarAreaController.js
378 kB   PieController.js
378 kB   RadarController.js
378 kB   ScatterController.js
378 kB   ArcElement.js
378 kB   LineElement.js
378 kB   PointElement.js
378 kB   BarElement.js
385 kB   Decimation.js
394 kB   Filler.js
397 kB   Legend.js
382 kB   SubTitle.js
382 kB   Title.js
384 kB   Tooltip.js
378 kB   CategoryScale.js
378 kB   LinearScale.js
378 kB   LogarithmicScale.js
378 kB   RadialLinearScale.js
378 kB   TimeScale.js
378 kB   TimeSeriesScale.js

After:

210 kB   Chart.js
83.7 kB  BarController.js
72.5 kB  BubbleController.js
80.2 kB  DoughnutController.js
74.1 kB  LineController.js
75.2 kB  PolarAreaController.js
80.4 kB  PieController.js
71.1 kB  RadarController.js
74.8 kB  ScatterController.js
40.4 kB  ArcElement.js
53.5 kB  LineElement.js
36 kB    PointElement.js
37 kB    BarElement.js
36.4 kB  Decimation.js
69.6 kB  Filler.js
71.1 kB  Legend.js
49.5 kB  SubTitle.js
49.6 kB  Title.js
85.8 kB  Tooltip.js
84.2 kB  CategoryScale.js
89.6 kB  LinearScale.js
92.6 kB  LogarithmicScale.js
106 kB   RadialLinearScale.js
95.4 kB  TimeScale.js
98.5 kB  TimeSeriesScale.js

Assign properties to classes is a side effect, so that's why tree-shaking didn't work. Changed it to static properties.

Also, some calls (class constructors) are marked as pure.

Also added size-limit to control sizes of specific exports. Action is failing cause it should be in the base branch to compare sizes.

dangreen avatar Jul 21 '22 20:07 dangreen

Using esbuild? I think most others can shake assigning class properties (it can have side effects, but usually does not). I'm not against changing to static properties, but its a breaking change.

kurkle avatar Jul 22 '22 04:07 kurkle

@kurkle webpack with terser (default minifier, actually terser make tree-shake). Why it's braking change ? Public API stays the same.

dangreen avatar Jul 22 '22 05:07 dangreen

Changing es version can break things.

kurkle avatar Jul 22 '22 06:07 kurkle

@kurkle we can add, for example, swc to transpilation

dangreen avatar Jul 22 '22 06:07 dangreen

my test bench: https://github.com/dangreen/chartjs-treeshaking-test

dangreen avatar Jul 22 '22 06:07 dangreen

@kurkle we can add, for example, swc to transpilation

swc and even babel transpile it to side-effected code 😕

dangreen avatar Jul 22 '22 06:07 dangreen

Using esbuild? I think most others can shake assigning class properties (it can have side effects, but usually does not)

https://github.com/terser/terser/issues/724

https://github.com/babel/babel/issues/5902

dangreen avatar Jul 22 '22 06:07 dangreen

@kurkle Ok, it's breaking change, but it's a desirable and valuable fix. So what is the release plan?

dangreen avatar Jul 22 '22 07:07 dangreen

I think this is one big thing to drive towards v4. I think we need to do 3.8.1 and 3.9 first. There has been some talk on moving to typescript in v4 (does not need to be full conversion IMO, just the transpilation stuff and some part as POC so things can be converted in steps). But this ts thing could pushed forward to v5 too.

kurkle avatar Jul 22 '22 08:07 kurkle

@kurkle Got it. Thank you!

dangreen avatar Jul 22 '22 08:07 dangreen

I'd be open to getting through 3.8.1 and 3.9 quickly so that we can start on v4 dev.

etimberg avatar Jul 22 '22 11:07 etimberg

Will review this evening my time.

etimberg avatar Aug 03 '22 18:08 etimberg

Personally, I have no objection to using static properties. They look to be fairly well supported and users can use something like Babel if they need to support older browsers. So I'd be in favor of this change

benmccann avatar Aug 03 '22 20:08 benmccann

We need to merge these in some sane order to avoid tough rebases

kurkle avatar Aug 03 '22 23:08 kurkle

@benmccann

users can use something like Babel if they need to support older browsers

I have the article about that 🙂

https://dev.to/cubejs/why-and-how-to-transpile-dependencies-of-your-javascript-application-3phf

dangreen avatar Aug 04 '22 09:08 dangreen

@kurkle I think it will better to merge before https://github.com/chartjs/Chart.js/pull/10525

dangreen avatar Aug 04 '22 09:08 dangreen

Going to merge this first, then #10525 can be rebased

etimberg avatar Aug 04 '22 13:08 etimberg