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

Add sideEffects flag #303

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

andrewLapshov
Copy link

Hello! I need to add sideEffects: false for correct tree-shaking of Webpack (docs).

@zpao
Copy link
Owner

zpao commented Aug 22, 2023

Please revert unnecessary changes before I can consider this.

@andrewLapshov
Copy link
Author

Ready

@zpao
Copy link
Owner

zpao commented Aug 23, 2023

Thanks! Ok if I'm reading this correctly, all that would change is that it would enable Webpack to remove QRCodeCanvas from your app bundle if you only used QRCodeSVG (for example)?

@andrewLapshov
Copy link
Author

Yes, without this flag webpack will always include whole package to final bundle.

@zpao
Copy link
Owner

zpao commented Aug 23, 2023

From what I'm reading, this marks the whole module as side effect free, but it's not quite true.

This does run on import with some module global state. Does this need to be cleaned up?

qrcode.react/src/index.tsx

Lines 169 to 181 in 15f4b93

// For canvas we're going to switch our drawing mode based on whether or not
// the environment supports Path2D. We only need the constructor to be
// supported, but Edge doesn't actually support the path (string) type
// argument. Luckily it also doesn't support the addPath() method. We can
// treat that as the same thing.
const SUPPORTS_PATH2D = (function () {
try {
new Path2D().addPath(new Path2D());
} catch (e) {
return false;
}
return true;
})();

@andrewLapshov
Copy link
Author

ChatGPT says you are right :)

Yes, the code below is a side effect. A side effect is a change in the state of a program or external environment that occurs as a result of executing certain code. In this case, the code performs operations to create and add a path to a Path2D object, which can change the state of the program or environment.

@zpao
Copy link
Owner

zpao commented Aug 23, 2023

I'm up for doing that differently. It can be a fn call that caches a global so it's not a side effect. I would like to see what's actually necessary to make your change work. A quick example even with #208 applied doesn't seem to have any changes in the generated bundle. See https://github.com/zpao/qrcode.react/tree/sideEffects-test.

@kachkaev
Copy link
Contributor

kachkaev commented May 27, 2024

Hmm IDK if SUPPORTS_PATH2D counts as a side-effect TBH. It does not set anything like globalThis.foo = 42. Running it zero times or 100 times does not change anything apart from the returned value.

If SUPPORTS_PATH2D is not needed, it will be tree-shaken away. If it is needed, its value will be assigned on module load. Looks safe to use with sideEffects: false. This flag can be handy for:

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