scaffold-eth / scaffold-eth-2

Open source forkable Ethereum dev stack
https://scaffoldeth.io
MIT License
1.19k stars 745 forks source link

Return useAccountBalance hook #829

Closed rin-st closed 2 months ago

rin-st commented 2 months ago

Description

Basically useAccountBalance is useBalance with added watch: boolean property, so it's not the same as it was before.

useAccountBalance was removed less than a month ago, but I think we should return it. Reason - watch property was removed from useBalance hook. So whenever we need to use watched balance outside of Balance component, we need to dance with all those query/block number hooks etc. It adds complexity to the code and is also not so obvious to newcomers how to configure it properly.

notes:

Additional Information

rin-st commented 2 months ago

Maybe call this new component with an explicit name like useWatchedBalance?

Thought about something like that, but with watch param it's not always "watched" :)

And maybe remove the watch parameter, because if you don't want it watched you can just use the regular Wagmi useBalance component, right?

For me looks strange to use two different hooks for two slightly different cases, and hence for me is better to use watch param. But let's wait what others think, probably I'm wrong.

carletex commented 2 months ago

Thanks for this @rin-st ! And all for the discussion.

I think having this makes sense, since wagmi removed the watch prop in most hooks. In #700 we abstracted for builders in our hooks so they can still use watch. But not with the useBalance one, so this PR makes sense.

Regarding the name, I don't have a strong opinion, but If I had to vote, I'd say:

:1st_place_medal: useWatchBalance (or useBalanceWatch useWatchedBalance). This will make a lot of sense if we remove the watch argument as Damu said (if you don't want to watch you can use wagmi's useBalance directly)

:2nd_place_medal: But I also like the current name useAccountBalance or useScaffoldBalance (consistent with other hooks names) if we want to keep the watch prop.

Anything works in my mind tho!

technophile-04 commented 2 months ago

🥇 useWatchBalance (or useBalanceWatch useWatchedBalance). This will make a lot of sense if we remove the watch argument as Damu said (if you don't want to watch you can use wagmi's useBalance directly)

Yup makes sense and less confusion, so let's remove acceptance of watch property from hook and call it useWatchBalance .

rin-st commented 2 months ago

Maybe call this new component with an explicit name like useWatchedBalance?

🥇 useWatchBalance (or useBalanceWatch useWatchedBalance). This will make a lot of sense if we remove the watch argument as Damu said (if you don't want to watch you can use wagmi's useBalance directly)

Yup makes sense and less confusion, so let's remove acceptance of watch property from hook and call it useWatchBalance

I'd vote for useScaffoldBalance, but if you think it's better to useWatchBalance let's do it! I'll update pr soon

rin-st commented 2 months ago

Renamed to useWatchBalance, removed watch param

rin-st commented 2 months ago

Thanks Shiv!

Regarding ee10269 but I intentionally added new type. I can't find a proper article to explain why I prefer that way, so I just copy-paste ChatGpt answer. I don't say it's the right way, just something to consider

question: why to add new type here import SomeType from 'some-lib'; type ComponentPropsType = SomeType;

answer: The line type ComponentPropsType = SomeType; in your TypeScript code is a way to define a new type alias named ComponentPropsType that directly references SomeType, which is imported from 'some-lib'. There are several reasons why you might want to define such a type alias:

  1. Simplicity and Clarity: Defining ComponentPropsType can make your code more readable and maintainable. If SomeType is used as a component's props type across various parts of your application, renaming it to something more descriptive like ComponentPropsType can make it clearer what the role of this type is in the context of your application.

  2. Abstraction: By aliasing SomeType as ComponentPropsType, you abstract away the specific details of the type implementation which is defined in 'some-lib'. This abstraction can be useful if you later decide to change SomeType to another type. You would only need to change the alias definition in one place rather than throughout your codebase.

  3. Extensibility: Even if the alias initially is a direct reference (as in your example), setting up an alias like this can make it easier to extend or modify the type later. For instance, you might want to add additional properties or methods to ComponentPropsType by extending SomeType or combining it with other types.

  4. Consistency: Using a type alias helps maintain consistency in naming conventions and type usage across your codebase, particularly if SomeType is a generic or less intuitively named type. This makes your code easier to understand and work with for other developers.

  5. Type Refinement: You may want to refine or restrict the imported type in certain ways that are specific to your application. While your current line of code doesn't modify SomeType, it sets the stage for such potential modifications.

In essence, adding a type alias like ComponentPropsType can provide a foundation for better code structure, future scalability, and clearer communication in your codebase.

technophile-04 commented 2 months ago

Regarding ee10269 but I intentionally added new type.

Ohh, I thought 95% you just removed & { watch : true } and forgot to inline the type and 5% it was intentional. So just removed it since it was just a small hook with very straightforward functionality (and not planning to extend it in the future)

But yeah my bad should have asked before removing, and also the points make sense!! Thanks for sharing!!