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

Fix tree shaking #661

Closed
wants to merge 6 commits into from
Closed

Fix tree shaking #661

wants to merge 6 commits into from

Conversation

jeetiss
Copy link
Contributor

@jeetiss jeetiss commented Jun 7, 2023

hello, folks

I fixed two tree-shaking issues with this PR

  • package.json in dist/esm overrides default one so webpack doesn't see sideEffects: false option and disables optimizations. I added this field.
  • webpack and other tools don't remove top-level function calls even when sideEffects: false is specified, so I added /*#__PURE__*/ annotation with babel plugin

size before:

  viem (cjs)
  Size limit:   60 kB
  Size:         59.77 kB with all dependencies, minified and gzipped
  Loading time: 1.2 s    on slow 3G
  Running time: 427 ms   on Snapdragon 410
  Total time:   1.6 s
  
  viem (tree-shaking check)
  Package size limit has exceeded by 16.75 kB
  Size limit:   1.5 kB
  Size:         18.25 kB with all dependencies, minified and gzipped
  Loading time: 357 ms   on slow 3G
  Running time: 203 ms   on Snapdragon 410
  Total time:   560 ms
  
  viem/chains
  Package size limit has exceeded by 1.76 kB
  Size limit:   1 kB
  Size:         2.76 kB with all dependencies, minified and gzipped
  Loading time: 54 ms   on slow 3G
  Running time: 98 ms   on Snapdragon 410
  Total time:   152 ms

size after:

  viem (cjs)
  Size limit:   60 kB
  Size:         59.77 kB with all dependencies, minified and gzipped
  Loading time: 1.2 s    on slow 3G
  Running time: 404 ms   on Snapdragon 410
  Total time:   1.6 s
  
  viem (tree-shaking check)
  Size limit:   1.5 kB
  Size:         1.43 kB with all dependencies, minified and gzipped
  Loading time: 28 ms   on slow 3G
  Running time: 83 ms   on Snapdragon 410
  Total time:   111 ms
  
  viem/chains
  Size limit:   1 kB
  Size:         993 B with all dependencies, minified and gzipped
  Loading time: 20 ms on slow 3G
  Running time: 76 ms on Snapdragon 410
  Total time:   96 ms

PR-Codex overview

This PR adds pure annotation to the codebase and updates some dependencies.

Detailed summary

  • Adds pure annotation to the codebase using the babel-plugin-annotate-pure-calls plugin.
  • Updates the @babel/cli and @babel/core dependencies.
  • Updates the @babel/plugin-syntax-typescript dependency.
  • Updates the size-limit configuration to include new size limits for different parts of the codebase.
  • Adds sideEffects to the package.json file to optimize tree-shaking.
  • Adds a new script to build a pure-annotations version of the codebase.
  • Makes minor changes to code formatting in various files.

The following files were skipped due to too many changes: src/chains.ts, pnpm-lock.yaml

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@changeset-bot
Copy link

changeset-bot bot commented Jun 7, 2023

⚠️ No Changeset found

Latest commit: e4ed9da

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jun 7, 2023

@jeetiss is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

@socket-security
Copy link

socket-security bot commented Jun 7, 2023

New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives1 Size Publisher
babel-plugin-annotate-pure-calls 🆕 0.4.0 None +17 9.06 MB andarist
@babel/core 🆕 7.22.1 None +16 9.06 MB nicolo-ribaudo
@babel/cli 🆕 7.21.5 filesystem +21 9.28 MB nicolo-ribaudo

Footnotes

  1. https://docs.socket.dev

@fubhy
Copy link
Collaborator

fubhy commented Jun 7, 2023

Hey there. Good find!

  • package.json in dist/esm overrides default one so webpack doesn't see sideEffects: false option and disables optimizations. I added this field.

Is that a bug in webpack? Doesn't hurt to add that property here but feels like webpack should traverse to & inherit that from the package.json of the package.

webpack and other tools don't remove top-level function calls even when sideEffects: false is specified, so I added /#PURE/ annotation with babel plugin

Instead of introducing another build step & babel for that, let's just add that annotation to the offending root level function calls in viem/chains and wherever else necessary?

@jeetiss
Copy link
Contributor Author

jeetiss commented Jun 7, 2023

Is that a bug in webpack?

I don't know exactly, but it seems as not. esbuild works the same way, and includes all classes when only defineBlock is imported https://bundlejs.com/?q=viem&treeshake=%5B%7B+defineBlock+%7D%5D&config=%7B%22esbuild%22%3A%7B%22minify%22%3Afalse%7D%7D

another build step & babel

I wouldn't say I like this either, but this solution is bulletproof and typescript doesn't provide any way to achieve the same result.

Current code contains 110 pure annotations:

Снимок экрана 2023-06-07 в 16 02 04

Any usage without annotations would include the function and the call to bundle, so I think is better to use an automated solution.

But ofc, I can set comments manually if you don't like the costs of babel

@fubhy
Copy link
Collaborator

fubhy commented Jun 7, 2023

Any usage without annotations would include the function and the call to bundle, so I think is better to use an automated solution.

Oh okay, so the annotation is needed for more than just the viem/chains use of defineChain?

@jeetiss
Copy link
Contributor Author

jeetiss commented Jun 7, 2023

yes, it is used for all define functions: defineBlock, defineTransaction, defineChain, defineFormatter, defineTransactionReceipt, defineTransactionRequest

Pure annotations affect viem and other entries too, but not as dramatically as viem/chains

@jxom
Copy link
Member

jxom commented Jun 9, 2023

Just gave this a test run, and it seems to strip out all the sourcemap comments. Can we stop it from doing that?

@fubhy
Copy link
Collaborator

fubhy commented Jun 9, 2023

@jeetiss Could you split this into two PRs? One for just adding the sideEffects: false to the package.jsons and the other for setting up the PURE annotations?

We are currently researching our options for that and don't have a super clear picture yet. A lot of the discussions both in the typescript repository are super recent and inconclusive:

We also found some interesting related discussions and recent activity for rollup, esbuild, etc. that are relevant:

I dislike the proposed babel solution because it feels somewhat icky to post-process the build output with an additional step effectively fiddling with the dist artifacts after build. I'm worried about how that might potentially affect source maps too.

My personal preference for the annotations would be:

  1. Add NO_SIDE_EFFECTS annotations to the declaration functions
  2. Add PURE annotations to the usage sites of these functions (until NO_SIDE_EFFECTS is widely supported)
  3. Make tsc play nice and emit those properly and also check that it doesn't produce any other code (e.g. classes) that are not considered side-effect free

This is my personal preference with admitedly only rather superficial understanding of the status quo of tooling support & best practices for these annotations in general.

@jeetiss
Copy link
Contributor Author

jeetiss commented Jun 9, 2023

@fubhy yes ofc, I will split this PR sideEffects: false gain most improvements here and are not much controversial as pure annotations.

I found that I can run babel over typescript sources, so I can do your suggestion automatically, it just adds pure to every top-level invocation

Instead of introducing another build step & babel for that, let's just add that annotation to the offending root level function calls in viem/chains and wherever else necessary?

It looks like a good compromise for me before NO_SIDE_EFFECTS is settled well

What do you think?

@fubhy
Copy link
Collaborator

fubhy commented Jun 9, 2023

I found that I can run babel over typescript sources, so I can do your suggestion automatically, it just adds pure to every top-level invocation

Yeah that sounds good. Adding them to the source code would also give us an idea how many of those are necessary and how much maintenance burden that would add going forward.

@jxom Also identified a potential problem with how tsc is emitting code for errors/base.ts. We'd have to check that too but that could also be another follow-up.

EDIT: That base error class issue is solved with this: #684

src/utils/rpc.ts Show resolved Hide resolved
src/utils/promise/withCache.ts Show resolved Hide resolved
src/utils/encoding/toBytes.ts Show resolved Hide resolved
src/utils/encoding/toHex.ts Show resolved Hide resolved
src/utils/encoding/toHex.ts Show resolved Hide resolved
src/utils/observe.ts Show resolved Hide resolved
src/utils/promise/createBatchScheduler.ts Show resolved Hide resolved
@jeetiss
Copy link
Contributor Author

jeetiss commented Jun 9, 2023

This PR contains a command to add pure annotations, if you want to run it again

close in favor of #687

@jeetiss jeetiss closed this Jun 9, 2023
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.

None yet

3 participants