Skip to content

feat: sideEffects false#10526

Merged
etimberg merged 2 commits into
chartjs:masterfrom
TrigenSoftware:feat-side-effects-false
Aug 6, 2022
Merged

feat: sideEffects false#10526
etimberg merged 2 commits into
chartjs:masterfrom
TrigenSoftware:feat-side-effects-false

Conversation

@dangreen

@dangreen dangreen commented Jul 28, 2022

Copy link
Copy Markdown
Collaborator

closes #10284

The problem was what Rollup was applying sideEffects: false to test build for Karma, which has side effects.

NOTE: need to merge #10504 and #10525 first

@kurkle

kurkle commented Jul 29, 2022

Copy link
Copy Markdown
Member

Isn't that the same build that would end up in cdn? If tests needs. treeshake: false, then the umd build would not be complete, right?

@dangreen

Copy link
Copy Markdown
Collaborator Author

@kurkle Yes. But here they marked as files with side effects.

@kurkle

kurkle commented Jul 30, 2022

Copy link
Copy Markdown
Member

I committed the dist/chart.js to a test repository for a diff. This is what changes on the output with these additions:
https://github.com/kurkle/chart-test/commit/df9562b9fcdbae96c7827d7c23a6fc38d395f5ea

@dangreen

Copy link
Copy Markdown
Collaborator Author

@kurkle Sorry. Fixed. I missed that we have one file with side effects in src.

@kurkle kurkle added this to the Version 4.0 milestone Aug 1, 2022

@benmccann benmccann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there any TODOs we should add to remove the side effects? I'm curious if you think there are follow-ups we should do to this

Comment thread package.json Outdated
kurkle
kurkle previously approved these changes Aug 4, 2022
@dangreen

dangreen commented Aug 5, 2022

Copy link
Copy Markdown
Collaborator Author

If you are worried about that side-effects from "core.scale.defaults" will every time in the bundle, I can calm down you: in PR with tree-shaking this problem is solved, you can test it on my test bench.

It's not true. defaults.set code will always be in the user's bundle because it is a side-effect.

Maybe we should just point to the entry in src/ or copy them from src/ to dist/ if needed

It will not help.

Is there a way that we can set the defaults in a side-effect free manner so that we don't have to include either file regardless?

I will try to make it

@dangreen

dangreen commented Aug 5, 2022

Copy link
Copy Markdown
Collaborator Author

I will try to make it

I think it doesn't make sense because this code will be in a bundle in most cases anyway.

@dangreen

dangreen commented Aug 5, 2022

Copy link
Copy Markdown
Collaborator Author

@kurkle Could you please restart CI? It has strange error.

@kurkle

kurkle commented Aug 5, 2022

Copy link
Copy Markdown
Member

@kurkle Could you please restart CI? It has strange error.

Restarted, though it might be due to other merges. Maybe a rebase is needed.

@dangreen

dangreen commented Aug 5, 2022

Copy link
Copy Markdown
Collaborator Author

master:

Снимок экрана 2022-08-05 в 19 57 50

"refactor: apply defaults by pure way":

Снимок экрана 2022-08-05 в 19 54 29

@benmccann

Copy link
Copy Markdown
Contributor

If you don't mind waiting a little bit, I'll send an alternative idea for review and we can decide which approach we might want to take. I'll do that in the next few hours

@benmccann

Copy link
Copy Markdown
Contributor

I started on making animations optional here: #10574. After digging into the code, it looks like this won't necessarily conflict with that work, so we could probably merge this and then I can just build on top of it

@etimberg
etimberg merged commit cdb17d6 into chartjs:master Aug 6, 2022
@dangreen
dangreen deleted the feat-side-effects-false branch August 16, 2022 08:01
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.

Add sideEffects: false to package.json

4 participants