Refactor: Token balances to fn component #319
Conversation
| }); | ||
| return get(tokenBalancesRequest, 'data', { hits: [], has_next: false }); | ||
| }; | ||
| const getTokenBalances = useCallback( |
There was a problem hiding this comment.
[question] You are declaring it with useCallback but you don't inform its dependencies, it is empty in the end, why? Shouldn't it include at least queryTokenId and searchAfter as they can change?
There was a problem hiding this comment.
These parameters are actually passed every time the function is called, and not obtained from the component state. See answer below for a more detailed explanation.
| [] | ||
| ); | ||
|
|
||
| const loadTokenBalanceInformation = useCallback(async queryTokenId => { |
There was a problem hiding this comment.
[question] Same issue, shouldn't it depend on queryTokenId?
There was a problem hiding this comment.
The way I structured this component, the useEffect() on line 168 is the one responsible for obtaining the most up-to-date queryTokenId and pass it as a parameter for this callback function.
Since the loadTokenBalanceInformation function will always receive all needed parameters directly through its calls, it never needs to be re-rendered because of any state changes, so its dependencies are empty.
I considered the possibility of removing it from inside the component altogether, but it needs to update the tokenBalanceInformationError state, so I left it inside the component. A more profound refactor could have done this, but I preferred not changing too much the logic of the component for now.
Note: the set functions are actually stable and need not be informed as dependencies for callbacks and effects ( see relevant docs )
| history.push(newURL); | ||
| }, | ||
| [history] |
There was a problem hiding this comment.
[question] Isn't it a circular dependency?
There was a problem hiding this comment.
It sure looks like one, but it isn't. This is a useCallback declaration, not an useEffect.
By having history as a dependency for this function, we only ensure that it always has the most up-to-date version of the history hook available whenever it's called. We are not running it every time the history changes.
See more on the official docs.
| const [tokenBalances, setTokenBalances] = useState([]); | ||
| const [hasAfter, setHasAfter] = useState(false); | ||
| const [hasBefore, setHasBefore] = useState(false); | ||
| const [sortBy, setSortBy] = useState(null); | ||
| const [order, setOrder] = useState(null); | ||
| const [page, setPage] = useState(1); | ||
| const [pageSearchAfter, setPageSearchAfter] = useState([]); | ||
| const [loading, setLoading] = useState(true); | ||
| const [isSearchLoading, setIsSearchLoading] = useState(false); | ||
| const [calculatingPage, setCalculatingPage] = useState(false); | ||
| const [error, setError] = useState(false); | ||
| const [tokenBalanceInformationError, setTokenBalanceInformationError] = useState(false); | ||
| const [transactionsCount, setTransactionsCount] = useState(0); | ||
| const [addressesCount, setAddressesCount] = useState(0); | ||
| const [tokensApiError, setTokensApiError] = useState(false); |
There was a problem hiding this comment.
[note] I don't know if this is a good idea because it can cause the component to trigger an unnecessary amount of re-renderings and it can cause other problems.
There was a problem hiding this comment.
React batches state changes made within a single synchronous iteration, so there will be only a single re-render for all those changes ( except on some very specific async situations, not the case in this component ).
I found this article that explains it way better than I could, with details, example codes and live tests.
Acceptance Criteria
TokenBalancescomponent should be refactored to functional componentNotes
As with the other recent functional component rewritings, it's easier to review by comparing the before and after than by looking at the diff directly.
Security Checklist