-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop using loader functions, part 1 #1306
Changes from all commits
415e2c8
44952d2
bb8e290
033323c
a46a4bc
8e1f732
7830835
fd2b9c8
e54075b
8cef12d
5f6c8b3
c21846a
e00cc1d
ca1b276
8b5c5fc
8601886
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@penumbra-zone/zquery': patch | ||
'minifront': patch | ||
--- | ||
|
||
Stop using loader functions for most routes; fix typings issue in ZQuery |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,8 +28,12 @@ const retry = async (fn: () => boolean, ms = 500, rate = Math.max(ms / 10, 50)) | |
* timeout. This is a temporary solution until loaders properly await Prax | ||
* connection. | ||
*/ | ||
export const abortLoader = async (): Promise<void> => { | ||
export const abortLoader = async (): Promise<null> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the trick, ie. returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, no — loaders are required to return something rather than |
||
await throwIfPraxNotInstalled(); | ||
await retry(() => isPraxConnected()); | ||
throwIfPraxNotConnected(); | ||
|
||
// Loaders are required to return a value, even if it's null. By returning | ||
// `null` here, we can use this loader directly in the router. | ||
return null; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,5 @@ | ||
import { LoaderFunction, useLoaderData } from 'react-router-dom'; | ||
import { AddressIcon } from '@repo/ui/components/ui/address-icon'; | ||
import { AddressComponent } from '@repo/ui/components/ui/address-component'; | ||
import { BalancesByAccount, getBalancesByAccount } from '../../../fetchers/balances/by-account'; | ||
import { | ||
Table, | ||
TableBody, | ||
|
@@ -11,20 +9,15 @@ import { | |
TableRow, | ||
} from '@repo/ui/components/ui/table'; | ||
import { ValueViewComponent } from '@repo/ui/components/ui/tx/view/value'; | ||
import { abortLoader } from '../../../abort-loader'; | ||
import { EquivalentValues } from './equivalent-values'; | ||
import { Fragment } from 'react'; | ||
import { shouldDisplay } from './helpers'; | ||
|
||
export const AssetsLoader: LoaderFunction = async (): Promise<BalancesByAccount[]> => { | ||
await abortLoader(); | ||
return await getBalancesByAccount(); | ||
}; | ||
import { useBalancesByAccount } from '../../../state/balances'; | ||
Comment on lines
-19
to
-22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that I've deleted the loaders for a bunch of routes, but I've replaced their loaders in |
||
|
||
export default function AssetsTable() { | ||
const balancesByAccount = useLoaderData() as BalancesByAccount[]; | ||
const balancesByAccount = useBalancesByAccount(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we were previously using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should be unnecessary - the loader check was only present because the loaders were suppressing the check done at page layout There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've actually moved @turbocrime are you saying I can remove them, then? |
||
if (balancesByAccount.length === 0) { | ||
if (balancesByAccount.data?.length === 0) { | ||
return ( | ||
<div className='flex flex-col gap-6'> | ||
<p> | ||
|
@@ -40,7 +33,7 @@ export default function AssetsTable() { | |
|
||
return ( | ||
<Table> | ||
{balancesByAccount.map(account => ( | ||
{balancesByAccount.data?.map(account => ( | ||
<Fragment key={account.account}> | ||
<TableHeader className='group'> | ||
<TableRow> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,23 @@ | ||
import { createHashRouter, redirect } from 'react-router-dom'; | ||
import { PagePath } from './metadata/paths'; | ||
import { Layout } from './layout'; | ||
import AssetsTable, { AssetsLoader } from './dashboard/assets-table'; | ||
import AssetsTable from './dashboard/assets-table'; | ||
import TransactionTable from './dashboard/transaction-table'; | ||
import { DashboardLayout } from './dashboard/layout'; | ||
import { TxDetails, TxDetailsErrorBoundary, TxDetailsLoader } from './tx-details'; | ||
import { TxDetails, TxDetailsErrorBoundary } from './tx-details'; | ||
import { SendLayout } from './send/layout'; | ||
import { SendAssetBalanceLoader, SendForm } from './send/send-form'; | ||
import { SendForm } from './send/send-form'; | ||
import { Receive } from './send/receive'; | ||
import { ErrorBoundary } from './shared/error-boundary'; | ||
import { SwapLayout } from './swap/layout'; | ||
import { SwapLoader } from './swap/swap-loader'; | ||
import { StakingLayout, StakingLoader } from './staking/layout'; | ||
import { StakingLayout } from './staking/layout'; | ||
import { IbcLoader } from './ibc/ibc-loader'; | ||
import { IbcLayout } from './ibc/layout'; | ||
import { abortLoader } from '../abort-loader'; | ||
import type { Router } from '@remix-run/router'; | ||
|
||
export const rootRouter = createHashRouter([ | ||
export const rootRouter: Router = createHashRouter([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add some comments somewhere on the high-level similarities and differences between react-router loaders and zquery? From what I can tell, both support:
In terms of their differences, this is where some more clarification would be helpful. Zquery anchors on the idea of global state management, where state can be updated and shared across an application, right? React-router on the other hand fetches data specifically for a single route, and rather than being stored globally, it's only accessible by a specific component. I guess I'm wondering where component-level data fetching, a staple of the latter case, would generally be more preferred? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, hm, I'm realizing I should write up an ADR for all of this. The idea is that we want to consolidate our state into one state machine, rather than having it split among several (Zustand, react-router, React Query, etc.). I would assume that component-level data fetching as provided by React Query is for simpler applications that don't use a global data store like Zustand. It's also useful for server-side applications that load data and render it before sending HTML down to the browser — unlike what we're doing with our entirely client-side rendering. |
||
{ | ||
path: '/', | ||
element: <Layout />, | ||
|
@@ -28,7 +30,7 @@ export const rootRouter = createHashRouter([ | |
children: [ | ||
{ | ||
index: true, | ||
loader: AssetsLoader, | ||
loader: abortLoader, | ||
element: <AssetsTable />, | ||
}, | ||
{ | ||
|
@@ -43,7 +45,7 @@ export const rootRouter = createHashRouter([ | |
children: [ | ||
{ | ||
index: true, | ||
loader: SendAssetBalanceLoader, | ||
loader: abortLoader, | ||
element: <SendForm />, | ||
}, | ||
{ | ||
|
@@ -59,13 +61,13 @@ export const rootRouter = createHashRouter([ | |
}, | ||
{ | ||
path: PagePath.TRANSACTION_DETAILS, | ||
loader: TxDetailsLoader, | ||
loader: abortLoader, | ||
element: <TxDetails />, | ||
errorElement: <TxDetailsErrorBoundary />, | ||
}, | ||
{ | ||
path: PagePath.STAKING, | ||
loader: StakingLoader, | ||
loader: abortLoader, | ||
element: <StakingLayout />, | ||
}, | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment in
root-router