search

emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.5k stars 1.11k forks source link

@emotion/react@11.0.0 is 10% bigger than @emotion/core@10.1.1 #2097

Open alexreardon opened 4 years ago

alexreardon commented 4 years ago

Hi friends,

I raise this with the upmost thanks and gratefulness for this project. I've noticed that @emotion/react@11.0.0 is +10% bigger than @emotion/core@10.1.1

Some thoughts:

I appreciate the sometimes projects need to get bigger in order to move forward well. But I wanted to raise this just in case there was something that could be done.

Also, i'm not sure what these badges are referring to in the readme:

Screen Shot 2020-11-13 at 1 10 50 pm
alexreardon commented 4 years ago

I wasn't sure what category for this issue was best, I went for 'bug' but feel free to change it to something else

ssslambda commented 4 years ago

I think, even if theming is in main package now, tree-shaking should do it's work.

Andarist commented 4 years ago

@alexreardon thank you for keeping an eye for things like this - it has actually helped me to improve the tree-shakeability of @emotion/react. The PR is not merged in yet but you can check it out here: https://github.com/emotion-js/emotion/pull/2101

I've prepared a demo that is showcasing how this change will improve the story around this. Notice how in this commit hoist-non-react-statics has completely vanished from webpack and Rollup bundles: https://github.com/Andarist/emotion-11-tree-shaking-test/commit/be06ae978615f949ddd0d18e00afbd250e2897f8 . This is the commit that applies the patch from the PR I've mentioned here.

That being said - I don't believe that bundlephobia is an ideal tool for measuring things like this. It's probably one of the best though and it would be very hard to create a better one. The problem is that it all depends on the usage and what can be dropped from a particular package. As I've showcased here it's totally possible to cut down the bytes that will be put into our consumers' bundle if they don't end up using withTheme export. Splitting a package (and thus decreasing the overall DX) just to satisfy metrics like bundlephobia's ones feels like a counter-productive thing to do. It's a good thing to care about bundlesizes etc and we certainly take this into account - in fact, we use several build techniques to cut down the final size (providing special browser build, NODE_ENV checks, using PURE annotations and more). I don't think though it's worth pursuing a vanity metric here if it doesn't reflect the actual usage.

I understand though that people just won't realize this subtlety, we have no means to communicate our intentions to bundlephobia and people will stop analyzing after seeing a single, simple, final score of the overall bundlesize. This is very unfortunate but I don't know how the situation can be improved here.

If we simply subtract the react-is + hoist-non-react-statics % involvement (based on bundlephobia's composition values) then we'll end up with 6.52kb (7.3kb * 0.894) which is an improvement over v10's 6.6kb and this already includes theme-related helpers in v11. I don't think this math is super accurate because % involvement is based on non-minified sizes but it shows that there is a high possibility of those bundlesizes being very close to each other.

Also, i'm not sure what these badges are referring to in the readme:

Those bagdes were pointing to the old package names - thanks for noticing. I've prepared a fixing PR for this here: https://github.com/emotion-js/emotion/pull/2099

Andarist commented 3 years ago

For some reason @emotion/react@11.1.0 has not fixed the issue with those unwanted deps being bundled in (even though I have been testing it) but after a quick investigation I've managed to fix this and @emotion/react@11.1.1 works as expected in this regard. You can checkout the updated demo here: https://github.com/Andarist/emotion-11-tree-shaking-test/commit/507baa4df207a287f82de5197844bb15e66739c4

alexreardon commented 3 years ago

Thanks for engaging with this. Sadly I cannot check it out until #2114 is fixed