Jon/fix/coinrank-loading #5544
Jon/fix/coinrank-loading #5544
Conversation
| if (lastSceneFiat !== coingeckoFiat) { | ||
| // Clear cache if we changed the fiat while outside of this scene | ||
| debugLog(LOG_COINRANK, 'Clearing coinRankingDatas cache') | ||
| coinRanking.coinRankingDatas = [] | ||
| lastStartIndex.current = 1 | ||
| setDataSize(0) | ||
| setRequestDataSize(QUERY_PAGE_SIZE) | ||
| } |
There was a problem hiding this comment.
Why not just put this in useMount hook?
There was a problem hiding this comment.
See comment
#5544 (comment)
We need to pass mounted.current around separately anyway, so might as well fold
the changes into this existing effect.
There was a problem hiding this comment.
Using useMount for the purpose of avoid the eslint-disable-next-line rule hack
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Why was this // eslint-disable-next-line react-hooks/exhaustive-deps added? Is there an alternative?
There was a problem hiding this comment.
This was the cause of the actual bug in the task - this cleanup fn is appropriate only for when the scene is still mounted, implying that the fiat changed while the scene was mounted, so clearing everything was appropriate.
It's inappropriate to run this cleanup when the scene unmounts, which was happening in the original implementation. We lose the cache in that case, which defeats the purpose of the cache.
There was a problem hiding this comment.
What's the reason for the eslint-disable-next-line rule hack though? Seeing this hack is a red flag. I'm trying to understand why it's necessary and perhaps why another approach wasn't taken.
| // Queries should update this, but just in case: | ||
| lastSceneFiat = coingeckoFiat |
There was a problem hiding this comment.
Why not put this in a useUnmount hook?
There was a problem hiding this comment.
See comment
#5544 (comment)
We need to pass mounted.current around separately anyway, so might as well fold
the changes into this existing effect.
There was a problem hiding this comment.
Why I suggest useUnmount is to avoid the disabled eslint rule hack.
This also requires keeping track of the scene's last known fiat in case it changes while the scene is unmounted, which should result in a clearing of the cache.
Jon-edge
force-pushed
the
jon/fix/coinrank-loading
branch
from
7539af4 to
e954364
Compare
Jon-edge
force-pushed
the
jon/fix/coinrank-loading
branch
from
e954364 to
7c1c8d7
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: