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

feat: tree-shakeable client types #314

Closed
wants to merge 1 commit into from
Closed

Conversation

tmm
Copy link
Member

@tmm tmm commented Apr 5, 2023

Allows Client type to be used with public and wallet actions for maximum tree shakeability.

const client = createClient({
  chain: mainnet,
  transport: http(),
})
const res = await readContract(client, {
  abi: seaportAbi,
  address: '0x',
  functionName: 'name',
})

Doesn't work with test actions right now since they have a more complex type set up (e.g. mode).

🤖 Generated by Copilot at 0dff606

Summary

🧪🔄🤝

This pull request enhances the compatibility and flexibility of the actions module by refactoring the functions to accept different types of clients from the clients module. It also adds tests for the actions module using the vitest framework. It affects the files in the src/actions directory and the new file src/actions/actions.test-d.ts.

Client type expands
Actions work with many kinds
Autumn of refactor

Walkthrough

  • Add tests for the actions module using the vitest framework in a new file actions.test-d.ts (link)
  • Refactor the actions module to accept more generic Client types instead of only PublicClient or WalletClient types, to make the actions more flexible and compatible with different types of clients (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Update the order of the generic type parameters in the getContract function and the GetContractParameters type to match the order of the Client type, which is TChain, TAccount, TTransport, for consistency and ease of use (link, link, link)
  • Update the order of the generic type parameters in the createPendingTransactionFilter function to match the order of the Client type, which is TChain, TTransport, for consistency and ease of use (link)
  • Add a comma after the trim import from the utils module in the getEnsAddress file to follow the code style and formatting rules (link)

@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2023

⚠️ No Changeset found

Latest commit: 0dff606

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 Apr 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
viem ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 5, 2023 9:45pm
viem-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 5, 2023 9:45pm

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Size Change: +4 B (0%)

Total Size: 56.5 kB

Filename Size Change
dist/abi.js 257 B +1 B (0%)
dist/chains.js 2.03 kB +4 B (0%)
dist/chunk-6MARLCNB.js 0 B -415 B (removed) 🏆
dist/chunk-ULUVNOX4.js 0 B -43 kB (removed) 🏆
dist/contract.js 438 B -1 B (0%)
dist/ens.js 416 B +3 B (+1%)
dist/test.js 506 B -1 B (0%)
dist/utils/index.js 1.2 kB -2 B (0%)
dist/wallet.js 294 B -1 B (0%)
dist/chunk-DCABQ353.js 43 kB +43 kB (new file) 🆕
dist/chunk-PGCRJLIZ.js 416 B +416 B (new file) 🆕
ℹ️ View Unchanged
Filename Size
dist/accounts/index.js 1.36 kB
dist/ethers.js 523 B
dist/index.js 5.54 kB
dist/public.js 457 B
dist/window.js 67 B

compressed-size-action

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #314 (0dff606) into main (5530af2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #314   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files         262      262           
  Lines       16808    16812    +4     
  Branches     1795     1795           
=======================================
+ Hits        16788    16792    +4     
  Misses         18       18           
  Partials        2        2           
Impacted Files Coverage Δ
src/actions/ens/getEnsAddress.ts 100.00% <100.00%> (ø)
src/actions/ens/getEnsAvatar.ts 100.00% <100.00%> (ø)
src/actions/ens/getEnsName.ts 100.00% <100.00%> (ø)
src/actions/ens/getEnsResolver.ts 100.00% <100.00%> (ø)
src/actions/ens/getEnsText.ts 100.00% <100.00%> (ø)
src/actions/getContract.ts 100.00% <100.00%> (ø)
src/actions/public/call.ts 100.00% <100.00%> (ø)
src/actions/public/createBlockFilter.ts 100.00% <100.00%> (ø)
src/actions/public/createContractEventFilter.ts 100.00% <100.00%> (ø)
src/actions/public/createEventFilter.ts 100.00% <100.00%> (ø)
... and 79 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@roninjin10
Copy link
Sponsor Contributor

What does tree-shakable types even mean? Aren't all types tree shaken essentially both in .js and .d.ts files?

@jxom jxom mentioned this pull request Apr 6, 2023
@tmm
Copy link
Member Author

tmm commented Apr 6, 2023

What does tree-shakable types even mean? Aren't all types tree shaken essentially both in .js and .d.ts files?

Yeah, the types aren't tree-shakeable. This PR allows you to create a plain Client and use it with actions.

const client = createClient({ transport: http() })
const res = await readContract(client, {
  abi: seaportAbi,
  address: '0x',
  functionName: 'name',
})

Right now, only "decorated" clients can be used with actions. By allowing folks to also use Client, no decorated actions make it into the final bundle.

@roninjin10
Copy link
Sponsor Contributor

roninjin10 commented Apr 6, 2023

oh sick! Yea I remember when I first looked at viem this action(client, ...params) api was all I saw and I don't think decorated clients existed. Was very surprised later when you went the route of slightly more ergonomic ux for some % of devs with client.action syntax over the hyper optimized code splitting based on how ya'll seemed to be thinking about viem

@roninjin10
Copy link
Sponsor Contributor

roninjin10 commented Apr 11, 2023

I don't think I represent the majority, but will mention I hate this. Two different ways to do the same thing.

If we had the tree shakable version that takes the object as it's first argument, adding dot syntax just to appease the autocompleters feels like a bad reason to add a second way of doing things.

Similarly here it feels like a bit of an over-optimization (especially considering how hard viem is already crushing competition here) to include this as a tree shaking optimization

What would make me not hate it would be if this was a seperate library maybe called @viem/core or @viem/internal and viem simply just consumed that library and added decorators maybe some other nice to haves. I would like that you pick 1 way of doing things when you install but still would find the two library options unnecessary for this.

Like I said I don't think I represent the majority opinion though

How big is the codesplitting win here btw if it's huge that would change my mind (though it would make me wish the dot syntax didn't exist)

@tmm
Copy link
Member Author

tmm commented Apr 11, 2023

Closing since this is stale. Will open in follow-up.

@tmm tmm closed this Apr 11, 2023
@tmm tmm deleted the tmm/tree-shakeable-client-types branch April 11, 2023 22:19
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.

2 participants