-
-
Notifications
You must be signed in to change notification settings - Fork 819
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: react tree shaking and non-react entrypoint tree shaking #1482
Conversation
The latest updates on your projects. Learn more about Vercel for Git โ๏ธ
|
for what it's worth - this also fixes my rollup build (which currently tree shakes almost all components) PS: if I just remove the |
The short answer is it was the only way to get tree shaking in React main entrypoint...something is triggering side effects, but I can't figure out what. I believe it has something to do with re-exports. evanw/esbuild#613 Given infinite time, I'd love to track down where the sideEffects are coming from, but the tools dont make it simple. The closest I've found is from Rollup. https://rollupjs.org/configuration-options/#experimentallogsideeffects |
@daKmoR here's an updated thread of things that would need to change so we can get rid of this nasty https://twitter.com/RogersKonnor/status/1686774869218770944 Heres the summed up version of things I've identified so far:
LocalizeController is a fairly simple fix. Mark it as pure, carry on. Decorators. This one's tough. Related: evanw/esbuild#649 The TLDR is we'd need an https://www.npmjs.com/package/vite-plugin-tree-shakable-decorators
EDIT: |
but it's not pure... it has a side effect for the observer you could put that in a setup function and then force every app to call it... but that is rather cumbersome ๐
yeah transpilation is alway fun as long as it works... not so fun if it doesn't ๐ NoteMind my ignorance but may I ask why tree shaking is so important? I am just guessing here... but is it because of a single big bundle for react? ๐ค could that be split out? just tossing ideas - feel free to ignore ๐ค |
@daKmoR Nailed it. This was all about the single entrypoint for React. It is technically already split out, we just don't show it in the docs. In talking with @claviska we've kind of determined that React users should probably have to do the same and cherry pick if they want the smallest possible bundle and to only use what they import. |
This is unfortunate, but we have to balance the complexity with the cost to solve this and the overall benefit to end users. Non-framework folks cherry pick the same way, so it's really no different โ just less idiomatic for React. |
those kind of decision are always hard ๐ imho the right call ๐ and if you ever find yourself with too much time at hand ๐ฌ then you can work on a new major version that is fully side effect free... maybe Chat GTP can do it for us in 2050 ๐คฃ |
Closing this. The complexity and increased overhead is not worth it. Will be removing the |
๐ฎโ๐จ I think I got the treeshaking stuff down. Brief checks with Vite + Webpack builds seem to work.
This does introduce the caveat if you import a Shoelace component and a React component, you're doubling your build size. But I think this is a fair tradeoff.