refactor: lend markets query refactoring part 2 #1434
Conversation
This reverts commit 4b2b827.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [api?.hydrated, chainId], |
There was a problem hiding this comment.
Please never disable this rule, it's always a bug waiting to happen
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [api?.hydrated, chainId], | |
| [api, api?.hydrated, chainId], |
| return useMemo(() => { | ||
| if (!api) return undefined | ||
|
|
||
| // If markets aren't found they throw an error, but we want to return undefined instead |
There was a problem hiding this comment.
Please don't do this, ignoring errors is bad. Why are we trying to retrieve a market if it's not found?
If the API is hydrated it should never throw an error.
There was a problem hiding this comment.
Because somebody might enter gibberish into the URL?
There was a problem hiding this comment.
I'd argue that should crash the page with an error (or give a proper 404), instead of being ignored which could lead to the page being left in loading state.
| api.lendMarkets | ||
| .getMarketList() | ||
| .filter((marketName) => !networks[chainId].hideMarketsInUI[marketName]) | ||
| .map((name) => [api.getLendMarket(name).addresses.controller as Address, name]), |
There was a problem hiding this comment.
The only reason we created the mapping like this was because we wanted to use a query.
If this is a normal hook, there is no reason to return the name only, we might as well return the class instance
There was a problem hiding this comment.
Why not a query, though? We only need the mapping for redirecting.
There was a problem hiding this comment.
I could not reliably add the hydration validation in the query, reactivity for api.hydrated is wonky. But a hook with useMemo and api?.hydrated seemed to work, although you said it's still wonky in some way.
There was a problem hiding this comment.
Yeah we need to properly use the state machine from React. Currently it works only when it re-renders for some other reasons. Changing a field of an object is not supported
| push(getCollateralListPathname(params)) | ||
| } | ||
| }, [isSuccess, market, params, push, rMarket]) | ||
| }, [api?.hydrated, market, params, push, rMarket]) |
There was a problem hiding this comment.
this effect isn't triggered properly 😿
if you visit a market that doesn't exist, the hydration finishes but the redirect takes an unspecified amount of time to happen (when the page is re-rendered for some other reason)
|
Might put this PR back to draft until hydration issues are better resolved |
Preparation work for future
useMintMarkethook, which will replace the mint market collateral Zustand store which in turn is needed to add support for unified support for addresses in URL.hydratedChainId.useLendMarketsis no more. There's only auseLendMarketMapping, which maps lend market controller addresses to the lend market ids. This hook is used internally foruseLendMarket, and alsouseTvl. It is knows thatuseTvlcould use prices API instead, but that's out of the scope of this ticket and I want to keep it small; that's for later.Because there's so many pages and redirects already I've also not added redirecting from market id names to controller names yet