thirdweb-dev / js

Best in class web3 SDKs for Browser, Node and Mobile apps
https://thirdweb.com
Apache License 2.0
454 stars 371 forks source link

Tree Shaking is not working #459

Closed MananTank closed 8 months ago

MananTank commented 1 year ago

The Problem

importing even the tiniest thing from SDK results in a TON of code being imported and results in an extremely huge bundle size

Example

Let's say we add this line to our application: -

import { ChainId } from '@thirdweb-dev/sdk/evm';

( above-shown code is taken from thirdweb.com codebase)


ChainId is a tiny object and I would expect that only a little bit of JS will be imported from thirdweb-sdk in the final bundle - But that's not what happens!

This is what happens in Next.js & Vite


Next.js

Areas marked with an arrow are loaded because of the above-shown import

image image



Vite ( vanilla JS )

JS chunks created after running the build command:

image

A LOT (200kB !!!) of unnecessary JS is bundled together and the below-shown chunks are loaded on the client

image



What I Expect

Only the imported code - ChainId object should have been bundled - because It's a simple object that does not have any dependencies.

If the tree shaking was working properly - only the below-shown object would be imported from thirdweb-sdk - nothing else

image

What I Get

A lot of unnecessary code (200kb+) gets bundled!


Checkout these Repos to replicate the issue


What's happening here?

I suspect there's something wrong with how the imports/exports are done or maybe how the package is built that is causing the tree shaking to not work properly. I highly recommend investigating this as soon as possible


Impact 🔥

Websites using thirdweb-sdk are loading a LOT more JS than required - including https://thirdweb.com/


MananTank commented 1 year ago

Looks like this issue is also in @thirdweb-dev/react

I did import { ChainId } from '@thirdweb-dev/react'; and same thing happened as detailed above

jnsdls commented 1 year ago

Yep, this is a long standing issue that we've been tracking but that sadly isn't a simple fix.

The fundamental way we're exporting and importing files within the packages probably have something to do with it, we also have some circular dependencies inside the packages/sdk package that likely are to blame for a lot of it.

We'd love deeper research into what is going on and how we could potentially fix this.

With all that said this is also an industry wide problem, lots of external dependencies rely on poly filling huge native packages to work in the first place and so we end up inheriting a lot of that.

Further investigation and PRs definitely appreciated here!

MananTank commented 1 year ago

I would love to dig deep into this and figure out what's the issue here - I don't see why this can't be fixed.

Can you clarify what you mean by "polyfilling native packages" - Can you give some examples? - I can start the investigation from there

jnsdls commented 1 year ago

here are some things I would do to start investigating this (in no particular order):

  1. the packages/sdk package has a definite problem with dependency cycles, easy to find by turning the eslint rule for it back on here: https://github.com/thirdweb-dev/js/blob/4cdd0bd6348494a256d7c6a2bdf8f7b5c20f6877/packages/sdk/.eslintrc.cjs#L5
  2. we use preconstruct as a build tool today, it utilizes rollup (2) under the hood, there may just be configurations that we are not taking advantage off - we could also potentially switch to using rollup (3?) directly if preconstruct is too limited for our usecase (other possible options: tsup (based on esbuild) / direct esbuild)
  3. we currently do not have good "cleanliness" when it comes to which files contain which util functions etc, and our export paths are often similar to export * from "./..." in a bunch of index.ts files in folders etc, I have a hunch this interferes with proper tree shaking (also encourages problems like dependency cycles (see 1.))
  4. we may just be bundling dependencies that we don't need to (can we define some things as externals?) - this won't help with tree shaking but overall bundle size potentially
  5. lots of dependencies rely on native node modules like "stream" "crypto" "buffer" etc that then need to be polyfilled at some level since developers will want to utilize these libraries in web-frontends where these aren't available - I believe this polyfilling (often-times directly bundled into dependencies or done by us) likely leads to at least bundle bloat if not more tree shaking issues.

There might be more, in general I would try to experiment with @thirdweb-dev/sdk directly first - I would advise the use of yarn push at the root level of monorepo (relies on yalc under the hood) for quick iteration cycles with reproduction cases.

I expect this to be non-trivial work FWIW.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jnsdls commented 1 year ago

this will be addressed in v4

kien-ngo commented 1 year ago

Hi @MananTank, I want to give a hand on this topic, to reduce the bundle size of the packages. At the moment, importing just the <ThirdwebProvider /> from the SDK will add ~700kb of code. Which places do you think we should be looking at? 354471785_6352233681486209_6384358492889319716_n

P/S: Back in March 30th, the the page size was below 500kb. I was even celebrating in Discord LOL https://discord.com/channels/834227967404146718/834227967404146721/1091180839037054986

MananTank commented 1 year ago

@kienngo98

The increased bundle size is due to introduction of new wallets and other code changes made since March

We are now actively focusing on improving the bundle size for all our packages. For example: I just opened this PR - https://github.com/thirdweb-dev/js/pull/1212 which improves the bundle size by 160kB by fixing the tree-shaking issue in safeWallet. I'll do same for other wallets that are having the tree-shaking issue - that should bring the bundle size to ~500kB

Beyond that, The main issue is that tree shaking in @thirdweb-dev/sdk is not working properly. So far - we have removed circular dependencies and removed top-level side effects but tree-shaking is still not fixed yet. I'm also looking into using tsup instead of preconstruct for bundling - which is also improving the bundle size.

If you want to debug the tree-shaking issue, I suggest below process:

kien-ngo commented 1 year ago

improves the bundle size by 160kB

Amazing!

kien-ngo commented 1 year ago

@MananTank Just an update for the sdk: I have managed to narrow down this culprit at: packages\sdk\src\evm\core\sdk.ts

It has ~46.4kB of unshakable code

Update: After removing all the content inside the sdk.ts file, the bundle size was still the same, which led me to the "true" culprit here, and that is packages\sdk\src\evm\core\classes\index.ts image

Conclusion: It's somewhere among these files: image

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jnsdls commented 9 months ago

hey @MananTank remember this one?

We're finally going to fix it!

jnsdls commented 8 months ago

addressed in #2356