refactor: #513 migrate <AllTrades/> to react-query. #515
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/synthetixio/synthetix-exchange/j7agpbq8b |
|
I didn't get any compile errors removing |
|
@jwineman awesome work! lgtm! There is just one minor formatting issue with prettier (if you run Later on, we can add something like |
|
Can't remove the |
| max: MAX_TRADES, | ||
| }); | ||
| for (let i = 0; i < trades.length; i++) { | ||
| if (!!settledTradesQuery.data) { |
There was a problem hiding this comment.
Am I allowed to mutate state inside of react-query before I return it or does this code need to be modified to be immutable?
There was a problem hiding this comment.
All of this settledTradesQuery logic is essentially duplicated from the myTrades.js duck. Is it worth consolidating down to a single function?
| fetchMyTradesRequest(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| type SettledTrade = { |
There was a problem hiding this comment.
Should I be getting this type from somewhere else?
|
@jwineman great work. The next thing to do is to remove the rest of the instances of "myTrades" that use the sagas. Reusing There is a new pattern which I'm using now (in Kwenta for queries) All queries reside in - Then, whenever you want to use If a couple of components on the same page need to use the same query, then perhaps their shared parent can own it (or put it in context so you don't have to drill down the props). Sometimes you can just use the cache... but it really depends, because you need a component to first trigger the query. Let me know if you have any questions. |
|
@jwineman lgtm! awesome work! 👍 👍 I've invited @dvd-schwrtz to have a look as well (especially around the chart stuff, since he was the one implementing it) |
Fixes #513.