refactor: AddressDetailExplorer to fn component #317
Conversation
| */ | ||
| function shouldUpdate(tx, checkToken) { | ||
| const arr = [...tx.outputs, ...tx.inputs]; | ||
| const { token: queryToken } = pagination.current.obtainQueryParams(); |
There was a problem hiding this comment.
question(if-minor): Should we ignore token?
Let's say a tx arrives for a token that is not the currently selected.
When we change the selected token to this token, will the results reflect the current state or a previous state that should be refreshed?
There was a problem hiding this comment.
We actually have some logic holes in this component. The way we're calling shouldUpdate(), with checkToken: false, ensures that shouldUpdate will always return false. So we never show the alert anyway.
However, to answer your question directly, when we change the token the onTokenSelectChanged method is called which basically resets the whole page, so the results would always be correct.
| * | ||
| * @return {boolean} True if should update the list, false otherwise | ||
| */ | ||
| function shouldUpdate(tx, checkToken) { |
There was a problem hiding this comment.
suggestion(if-minor): Move this method outside the useEffect.
If should also change to receive the address, this will make the contents of the useEffect cleaner.
There was a problem hiding this comment.
Good point, this refactoring clarification also made a bug stand out ( see comment above ).
| * Will discard any errors and always set the metadataLoaded to true, even without any data. | ||
| * @type {(function(*): Promise<void>)|*} | ||
| */ | ||
| const getSelectedTokenMetadata = useCallback(async tokenToFetch => { |
There was a problem hiding this comment.
useCallback is meant to cache a function definition between re-renders.
Since this function has no dependencies, why do we need the useCallback?
There was a problem hiding this comment.
This function is indirectly used by an effect:
- getSelectedTokenMetadata
- reloadTokenSummaryAndHistory
- reloadTokensForAddress
- useEffect on line 321
To be able to use a function within an effect, we need to define it as a callback. I believe we could restructure this component in an even better way, but I didn't want to build additional components for the purpose of this refactor only.
This PR is my proposal between a full restructure and the simplest refactor.
| }); | ||
| return; | ||
| } | ||
| const getHistoryData = useCallback( |
There was a problem hiding this comment.
This callback requires txCache but it is the only one to call setTxCache.
Since this updates the txCache it I think we should not use setTxCache and just update the txCache directly.
The idea would be that the txCache is never "set", meaning that the same object will be used between re-renders.
This method adds transactions to the object and the next re-render will have the same object.
What do you think?
There was a problem hiding this comment.
State immutability is an important concept for React. By always using set and never mutating the object we can benefit from some features like "time machine" when debugging our application.
However, when studying some other alternative impelmentations for this getHistoryData based on your comment, I found a bug that could erase some transactions when under race conditions.
Fixed it on f979f47
| setLoadingSummary(false); | ||
| return undefined; | ||
| }, | ||
| [getHistoryData, getSelectedTokenMetadata] |
There was a problem hiding this comment.
Both of these dependencies can be a simple function, if they are changed to be a funcion maybe this should too.
There was a problem hiding this comment.
getHistoryData() gets called independently of getSelectedTokenMetadata() within onNextPageClicked() and onPreviousPageClicked().
I think that's why both these functions are separated: there's no need to make an additional metadata call on page change.
tuliomir
force-pushed
the
refactor/address-detail-fn
branch
from
0ecf5cb
to
f979f47
Compare
Acceptance Criteria
Motivation
This component used the deprecated
withRouterconnector, which will become unavailable in the coming upgrade to React Router v6.In order to remove its usage, it was necessary to refactor it to use react hooks as a functional component. This proved to be harder than initially foreseen because the code relied heavily upon async state setters, a feature that is also no longer available with
useState()hooks.So, instead of setting the state, waiting for it to be rendered and then applying the next set of instructions, the code now sets the state and then a
useEffect()handles its change. Many data flow reorganizations were made to achieve this new structure.Note for the reviewers
Since this component is so complex and had deep changes to its data flow, I'd recommend checking the before and after versions of the code to understand the changes made.
Security Checklist