Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
React redux implementation, fetching current price of BTC #245
React redux implementation, fetching current price of BTC #245
Changes from all commits
325b952
e1a31a1
1c58875
ef6d159
553fb98
bcf54d4
6e5ad8f
b88ed15
7628f66
9390ad9
3f07d13
88171fe
9c99552
2426636
49a0419
f3ac5a3
9861fe6
6b01f89
33362be
a6bbfd7
ab1db58
cb3d8a4
2efebb8
7062ced
2594155
fee4422
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is the correct approach in this case. 🤔 It seems to me that we should pass here only what we need. Without that
useCurrencyConversion
has access to other properties anyway.cc @r-czajkowski
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this is a classic approach and there is nothing wrong with it as long as the object contains the necessary data of a certain type. Objects in JavaScript are passed by reference, so in terms of performance, there is nothing wrong with it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking comment.
Maybe too late but I would consider using TanStack Query instead of redux at least for this feature. We can cache API response and we can set a time when the data should be re-fetched etc see https://tanstack.com/query/latest/docs/framework/react/reference/useQuery especially:
staleTime
andgcTime
options. My point is that if we add something else tobtc
store we need to duplicate the logic for loading and errors indicator and we have a lot of boilerplate to create a simple feature such as fetching USD price.We could use redux to store data that should be updated based on the emitted on-chain events but it should probably be possible with this lib as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, it looks good, but I wonder if instead of
TanStack Query
we should think about using of addon to the already installed redux-toolkit package: https://redux-toolkit.js.org/tutorials/rtk-query. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good too. But I would go with TanStack Query - it looks cool and powerful, is well-documented, there are a lot of examples, and is popular and well-maintained (38k stars on GitHub, only 30 issues and 18 opened pull requests). We can achieve all the stuff we need with TanStack Query w/o redux and we can easily plugin GraphQl which will be needed once we create our subgraph for Acre network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a task to deal with TanStack Query: #277