Extrapolate smart logic into containers - stop mem leak #1365
Conversation
|
|
|
||
| return <TradePriceMod {...props} fiatValue={fiatValueFormatted} /> | ||
| export default function TradePrice(props: TradePriceProps) { | ||
| return <TradePriceMod {...props} /> |
There was a problem hiding this comment.
Why not exporting this directly if u don't modify anything?
There was a problem hiding this comment.
artifact, i am removing this. before i had logic in here but moved it away, good catch
| @@ -186,7 +186,7 @@ export function useCoingeckoUsdValue(params: CoinGeckoUsdValueParams) { | |||
| return useGetPriceQuote({ ...coingeckoUsdPrice, currencyAmount }) | |||
| } | |||
|
|
|||
| export function useHigherUSDValue(currencyAmount: CurrencyAmount<Currency> | undefined) { | |||
| export function useHigherUSDValue(currencyAmount: CurrencyAmount<Currency> | undefined | null) { | |||
There was a problem hiding this comment.
Do we really need to have both undefined and null? what is the meaning of each?
Makes API of this hook harder imo
There was a problem hiding this comment.
no we don't made this for ease of typing as it's a bit random (Uni's fault hoenstly) as they have TYPE | undefined or TYPE | null everywhere it's wild
| {trade && ( | ||
| <Price trade={trade} theme={theme} showInverted={showInverted} setShowInverted={setShowInverted} /> | ||
| )} | ||
| <Price trade={trade} theme={theme} showInverted={showInverted} setShowInverted={setShowInverted} /> |
There was a problem hiding this comment.
But why? I would think the fault of memory leaks would be inside Price not in SwapMod. What is the issue of conditionally rendering a component?
If you change it for memory leaks here, u are doing a "white box" fix, where u know the implementation of Price does something bad when unmounted. I understand SwapMod wouldn't need to know anything about the Price component internals (should treat it as a "black box").
Let me know if my reasoning above is incorrect, likely missing sth here.
There was a problem hiding this comment.
Because price component was rendering a child that was calculating state that when un mounted was not cancelling the asynchronous promise
There was a problem hiding this comment.
This is what i mean, the problem is in price, not in SwapMod, isn't it?
There was a problem hiding this comment.
i see it 2 ways:
- SwapMod has the data sources/biz logic, and Price is dumb. It gets all the props etc, but now it needs a wrapping container to be independent and reusable elsewhere - SwapMod is the smart container in this example and relies on it to render it it's smart props
- it has a closer related wrapping container that isn't SwapMod that handles the biz logic and can be used as 1 component in other places (which is what i assume is what u intended with the refactor) - so now inside
/custom/swap/index.tswe have the logic inside the parent containers that allow the entire components to be reusable. for Price you can see that logic here: https://github.com/gnosis/cowswap/pull/1365/files#diff-e1d5a26ef1f0d5d518088887aa8e77620e509679ab3a6138b94360dc722102ccR196
which we can export and use everywhere and it will always render the price in USD
does that make sense?
| currencyAmount?: CurrencyAmount<Currency> | ||
| currencyAmount: CurrencyAmount<Currency> | undefined |
There was a problem hiding this comment.
potato / potahto ? (in other words, same thing, change needed?
W3stside
force-pushed
the
extrapolate-smart-logic-into-containers
branch
from
8fead7c to
34b06fe
Compare
|
@anxolin I'd like to move this forward, please re-review when you can |
…logic-into-containers
|
updated with develop but closing for now as not seeing the unmount error in barn/staging |
Summary
There was smart logic (hooks, async logic etc) causing memory leaks when these components were being unmounted conditionally inside of SwapMod.
Examples: TradeBasicDetails, RowFee, RowSlippage whom all calculated USD estimation inside their funciton body but were being unmounted inside SwapMod. Now the logic is moved to the relevant parent containers and unmounted inside the render method to safely avoid mem leaks.
To Test