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

Return useAccountBalance hook #829

Merged
merged 6 commits into from May 1, 2024
Merged

Conversation

rin-st
Copy link
Collaborator

@rin-st rin-st commented Apr 29, 2024

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:

  • we can probably use another hook name to not confuse with the previous version
  • default watch value is true since I think it is needed in most cases

Additional Information

Copy link
Collaborator

@damianmarti damianmarti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very great to me!!

It makes the Balance and FauceButton components a lot simpler and cleaner.

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

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

@rin-st
Copy link
Collaborator Author

rin-st commented Apr 30, 2024

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.

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rin-st !! I think this makes sense !

For naming, lol maybe we should call it useScaffoldBalalnce ? (denoting tiny wrapper around wagmi hook as our other hooks are) ?

Lol but yeah useAccountBalance also makes sense, lets hear from @carletex too on naming 🙌

packages/nextjs/hooks/scaffold-eth/useAccountBalance.ts Outdated Show resolved Hide resolved
packages/nextjs/hooks/scaffold-eth/useAccountBalance.ts Outdated Show resolved Hide resolved
packages/nextjs/hooks/scaffold-eth/useAccountBalance.ts Outdated Show resolved Hide resolved
@carletex
Copy link
Member

carletex commented May 1, 2024

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:

🥇 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)

🥈 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
Copy link
Collaborator

🥇 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
Copy link
Collaborator Author

rin-st commented May 1, 2024

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
Copy link
Collaborator Author

rin-st commented May 1, 2024

Renamed to useWatchBalance, removed watch param

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rin-st !!!

@technophile-04 technophile-04 merged commit e08fca0 into main May 1, 2024
1 check passed
@technophile-04 technophile-04 deleted the return-use-account-balance branch May 1, 2024 12:13
@rin-st
Copy link
Collaborator Author

rin-st commented May 1, 2024

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
Copy link
Collaborator

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!!

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

4 participants