Skip to content
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

fix: treeshaking #10504

Merged
merged 5 commits into from
Aug 4, 2022
Merged

fix: treeshaking #10504

merged 5 commits into from
Aug 4, 2022

Conversation

dangreen
Copy link
Collaborator

@dangreen dangreen commented Jul 21, 2022

fixes #10163

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.

@kurkle
Copy link
Member

kurkle commented Jul 22, 2022

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.

@dangreen
Copy link
Collaborator Author

dangreen commented Jul 22, 2022

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

@kurkle
Copy link
Member

kurkle commented Jul 22, 2022

Changing es version can break things.

@dangreen
Copy link
Collaborator Author

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

@dangreen
Copy link
Collaborator Author

@dangreen
Copy link
Collaborator Author

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

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

@dangreen
Copy link
Collaborator Author

dangreen commented Jul 22, 2022

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

terser/terser#724

babel/babel#5902

@dangreen
Copy link
Collaborator Author

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

@kurkle
Copy link
Member

kurkle commented Jul 22, 2022

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.

@dangreen
Copy link
Collaborator Author

@kurkle Got it. Thank you!

@kurkle kurkle added this to the Version 4.0 milestone Jul 22, 2022
@etimberg
Copy link
Member

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

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Needs a consensus we want to move to static fields. I'm game.

Need some note(s) to migration guide, as the defaults move up from prototype and someone might be using those.

Edit: also needs a rebase

@etimberg
Copy link
Member

etimberg commented Aug 3, 2022

Will review this evening my time.

@etimberg etimberg self-requested a review August 3, 2022 18:15
@benmccann
Copy link
Contributor

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

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Have one concern with the category scale change

@kurkle
Copy link
Member

kurkle commented Aug 3, 2022

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

@dangreen
Copy link
Collaborator Author

dangreen commented Aug 4, 2022

@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
Copy link
Collaborator Author

dangreen commented Aug 4, 2022

@kurkle I think it will better to merge before #10525

@etimberg
Copy link
Member

etimberg commented Aug 4, 2022

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

@etimberg etimberg merged commit a4de430 into chartjs:master Aug 4, 2022
@dangreen dangreen deleted the fix-treeshaking branch August 4, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree shaking not working as expected?
4 participants