-
Notifications
You must be signed in to change notification settings - Fork 15
refactor: new useQueryTable with legit types, returns the query data
#2567
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
Changes from all commits
83f6c8d
4de0bb0
9406444
8cab460
1a5fbda
ca6f430
65bb5fd
fee4c3f
5756dae
31ddca8
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 |
|---|---|---|
|
|
@@ -21,13 +21,15 @@ import { | |
| type UseQueryOptions, | ||
| type UseQueryResult, | ||
| } from '@tanstack/react-query' | ||
| import * as R from 'remeda' | ||
| import { type SetNonNullable } from 'type-fest' | ||
|
|
||
| import { invariant } from '~/util/invariant' | ||
|
|
||
| import type { ApiResult } from './__generated__/Api' | ||
| import { processServerError, type ApiError } from './errors' | ||
| import { navToLogin } from './nav-to-login' | ||
| import { type ResultsPage } from './util' | ||
|
|
||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| export type Params<F> = F extends (p: infer P) => any ? P : never | ||
|
|
@@ -123,6 +125,66 @@ export const getApiQueryOptions = | |
| ...options, | ||
| }) | ||
|
|
||
| // Managed here instead of at the display layer so it can be built into the | ||
| // query options and shared between loader prefetch and QueryTable | ||
| export const PAGE_SIZE = 25 | ||
|
|
||
| /** | ||
| * This primarily exists so we can have an object that encapsulates everything | ||
| * useQueryTable needs to know about a query. In particular, it needs the page | ||
| * size, and you can't pull that out of the query options object unless you | ||
| * stick it in `meta`, and then we don't have type safety. | ||
| */ | ||
| export type PaginatedQuery<TData> = { | ||
| optionsFn: ( | ||
| pageToken?: string | ||
| ) => UseQueryOptions<TData, ApiError> & { queryKey: QueryKey } | ||
| pageSize: number | ||
| } | ||
|
|
||
| /** | ||
| * This is the same as getApiQueryOptions except for two things: | ||
| * | ||
| * 1. We use a type constraint on the method key to ensure it can | ||
| * only be used with endpoints that return a `ResultsPage`. | ||
| * 2. Instead of returning the options directly, it returns a paginated | ||
| * query config object containing the page size and a function that | ||
| * takes `limit` and `pageToken` and merges them into the query params | ||
| * so that these can be passed in by `QueryTable`. | ||
| */ | ||
| export const getListQueryOptionsFn = | ||
| <A extends ApiClient>(api: A) => | ||
| < | ||
| M extends string & | ||
| { | ||
| // this helper can only be used with endpoints that return ResultsPage | ||
| [K in keyof A]: Result<A[K]> extends ResultsPage<unknown> ? K : never | ||
| }[keyof A], | ||
| >( | ||
| method: M, | ||
| params: Params<A[M]>, | ||
| options: UseQueryOtherOptions<Result<A[M]>, ApiError> = {} | ||
| ): PaginatedQuery<Result<A[M]>> => { | ||
| // We pull limit out of the query params rather than passing it in some | ||
| // other way so that there is exactly one way of specifying it. If we had | ||
| // some other way of doing it, and then you also passed it in as a query | ||
| // param, it would be hard to guess which takes precedence. (pathOr plays | ||
| // nice when the properties don't exist.) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const limit = R.pathOr(params as any, ['query', 'limit'], PAGE_SIZE) | ||
| return { | ||
| optionsFn: (pageToken?: string) => { | ||
| const newParams = { ...params, query: { ...params.query, limit, pageToken } } | ||
| return getApiQueryOptions(api)(method, newParams, { | ||
| ...options, | ||
| // identity function so current page sticks around while next loads | ||
| placeholderData: (x) => x, | ||
|
Collaborator
Author
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 just remembered the problem with this, which made me reluctant to do it everywhere, though doing it in just a few random tables (what we currently do) is probably even worse. When we don't do the blank flash, we don't scroll to top when the next page loads. 2024-11-21-pag-no-scroll.mp4
Collaborator
Author
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. GOT EM 2024-11-21-fix-table-scroll-to-top.mp4 |
||
| }) | ||
| }, | ||
| pageSize: limit, | ||
| } | ||
| } | ||
|
|
||
| export const getUseApiQuery = | ||
| <A extends ApiClient>(api: A) => | ||
| <M extends string & keyof A>( | ||
|
|
@@ -140,7 +202,7 @@ export const getUsePrefetchedApiQuery = | |
| options: UseQueryOtherOptions<Result<A[M]>, ApiError> = {} | ||
| ) => { | ||
| const qOptions = getApiQueryOptions(api)(method, params, options) | ||
| return ensure(useQuery(qOptions), qOptions.queryKey) | ||
| return ensurePrefetched(useQuery(qOptions), qOptions.queryKey) | ||
| } | ||
|
|
||
| const prefetchError = (key?: QueryKey) => | ||
|
|
@@ -152,7 +214,11 @@ Ensure the following: | |
| • request isn't erroring-out server-side (check the Networking tab) | ||
| • mock API endpoint is implemented in handlers.ts` | ||
|
|
||
| export function ensure<TData, TError>( | ||
| /** | ||
| * Ensure a query result came from the cache by blowing up if `data` comes | ||
| * back undefined. | ||
| */ | ||
| export function ensurePrefetched<TData, TError>( | ||
| result: UseQueryResult<TData, TError>, | ||
| /** | ||
| * Optional because if we call this manually from a component like | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ import { Outlet, useNavigate, type LoaderFunctionArgs } from 'react-router-dom' | |
|
|
||
| import { | ||
| apiQueryClient, | ||
| getListQFn, | ||
| queryClient, | ||
| useApiMutation, | ||
| useApiQueryClient, | ||
| useApiQueryErrorsAllowed, | ||
|
|
@@ -25,7 +27,7 @@ import { confirmDelete } from '~/stores/confirm-delete' | |
| import { SkeletonCell } from '~/table/cells/EmptyCell' | ||
| import { useColsWithActions, type MenuAction } from '~/table/columns/action-col' | ||
| import { Columns } from '~/table/columns/common' | ||
| import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable' | ||
| import { useQueryTable } from '~/table/QueryTable2' | ||
| import { Badge } from '~/ui/lib/Badge' | ||
| import { CreateLink } from '~/ui/lib/CreateButton' | ||
| import { EmptyMessage } from '~/ui/lib/EmptyMessage' | ||
|
|
@@ -52,12 +54,12 @@ const EmptyState = () => ( | |
| /> | ||
| ) | ||
|
|
||
| const snapshotList = (project: string) => getListQFn('snapshotList', { query: { project } }) | ||
|
Collaborator
Author
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 can now share the query options between the prefetch in the loader and the query table, and we don't have to specify |
||
|
|
||
| SnapshotsPage.loader = async ({ params }: LoaderFunctionArgs) => { | ||
| const { project } = getProjectSelector(params) | ||
| await Promise.all([ | ||
| apiQueryClient.prefetchQuery('snapshotList', { | ||
| query: { project, limit: PAGE_SIZE }, | ||
| }), | ||
| queryClient.prefetchQuery(snapshotList(project).optionsFn()), | ||
|
|
||
| // Fetch disks and preload into RQ cache so fetches by ID in DiskNameFromId | ||
| // can be mostly instant yet gracefully fall back to fetching individually | ||
|
|
@@ -100,7 +102,6 @@ const staticCols = [ | |
| export function SnapshotsPage() { | ||
| const queryClient = useApiQueryClient() | ||
| const { project } = useProjectSelector() | ||
| const { Table } = useQueryTable('snapshotList', { query: { project } }) | ||
| const navigate = useNavigate() | ||
|
|
||
| const { mutateAsync: deleteSnapshot } = useApiMutation('snapshotDelete', { | ||
|
|
@@ -132,6 +133,11 @@ export function SnapshotsPage() { | |
| [deleteSnapshot, navigate, project] | ||
| ) | ||
| const columns = useColsWithActions(staticCols, makeActions) | ||
| const { table } = useQueryTable({ | ||
| query: snapshotList(project), | ||
| columns, | ||
| emptyState: <EmptyState />, | ||
| }) | ||
| return ( | ||
| <> | ||
| <PageHeader> | ||
|
|
@@ -146,7 +152,7 @@ export function SnapshotsPage() { | |
| <TableActions> | ||
| <CreateLink to={pb.snapshotsNew({ project })}>New snapshot</CreateLink> | ||
| </TableActions> | ||
| <Table columns={columns} emptyState={<EmptyState />} /> | ||
| {table} | ||
| <Outlet /> | ||
| </> | ||
| ) | ||
|
|
||
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.
What this does: