fix: fetch price immediately after currency changes #1276
Conversation
There was a problem hiding this comment.
We want to only await the promise in the case where the currency was updated. As the comment mentioned, this is else on the SSR critical path so this would stall requests until the Coinbase API (over which we have no control) responds.
So I think you need a flag that you set in the useEffect query to await the price in this special case here.
|
OK thank you for the feedback, I have included a new flag |
* rename changedCurrency to fromCache * remove duplicate logic in getPrice * keep old cache code with comment about SSR critical path
There was a problem hiding this comment.
You forgot to update the GraphQL type definition and query so changedCurrency never arrived at the server. I fixed that in 4fb250c.
I hope you don't mind but I also refactored the code in 5ef94ad since there was some duplicate if (changedCurrency) ... code. I also renamed changedCurrency to fromCache to reflect better what this variable is used for.
However, I noticed that refetch not only refetches the data with new variables, but also sets these new variables for following polls.
So this mean if we set fromCache: false in the refetch, the price polls will from now on also use fromCache: false which is not intended...
Not sure what's the best way to fix this unintended interaction between refetching and polling.
Tbh, I suggest you work on something else since this seems to indeed be more complicated than fixing this bug is relevant.
Description
as mentioned by @ekzyis here, when we change the currency in settings, we don't immediately query the price, we wait for 30 seconds before doing so, which leaves us with a new currency symbol but with the old value. this PR aims to fix that.
Screenshots
Additional Context
Checklist
Are your changes backwards compatible? Please answer below:
Yes
Did you QA this? Could we deploy this straight to production? Please answer below:
Yes
For frontend changes: Tested on mobile? Please answer below:
Did you introduce any new environment variables? If so, call them out explicitly here:
No