Skip to content
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

refactor: start getting rid of apiQueryClient #2597

Merged
merged 9 commits into from
Dec 6, 2024

Conversation

david-crespo
Copy link
Collaborator

#2566 added a more flexible and powerful way of making typesafe API calls with React Query, but didn't convert things like invalidateQueries and setQueryData to use the new style to avoid churn. But we do eventually want to do that and get rid of apiQueryClient entirely. This PR aims to figure out what that will look like.

Copy link

vercel bot commented Dec 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Dec 6, 2024 7:06pm

// TODO: figure out if this is invalidating as expected, can we leave out the query
// altogether, etc. Look at whether limit param matters.
apiQueryClient.invalidateQueries('projectList')
invalidate('projectList')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, ideally we could do this as something like this:

queryClient.invalidateQueries(projectList.optionsFn())

or (to be clearer about what's going on)

queryClient.invalidateQueries({ queryKey: projectList.optionsFn().queryKey })

The problem here is that that queryKey has way too much specificity. Even without a page token specified, it will have the limit on it:

console/app/api/hooks.ts

Lines 168 to 169 in e4f70b0

optionsFn: (pageToken?: string) => {
const newParams = { ...params, query: { ...params.query, limit, pageToken } }

which means it won't invalidate any other fetches for the project list that have a different limit, and it won't invalidate any cached queries for pages other than the first one. This shows the wisdom of our approach of just using the method key alone without any parameters, which has the effect of invalidating any cached projectList fetch, regardless of the details.

I have an idea, though. If the list option object has another method that returns just the method key, we could use that.

project
)
const queryOptions = apiq('projectView', { path: { project: project.name } })
queryClient.setQueryData(queryOptions.queryKey, project)
Copy link
Collaborator Author

@david-crespo david-crespo Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised to discover this does actually enforce the type of project matches the endpoint. setQueryData is inferring it from the type of the options object.

image
image
image

this.invalidateQueries({ queryKey: [method] })
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach feels good, much better than a separate invalidate function (which I tried in 622bc80) or a separate apiQueryClient, like we have now. I like that it adds a new method and does not touch existing methods.

// avoid the project fetch when the project page loads since we have the data
queryClient.setQueryData('projectView', { path: { project: project.name } }, project)
const { queryKey } = projectView({ project: project.name })
queryClient.setQueryData(queryKey, project)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is kind of cool. Even though it's two lines, it's much more explicit about what's going on.

@@ -23,29 +18,31 @@ import { getProjectSelector, useProjectSelector } from '~/hooks/use-params'
import { addToast } from '~/stores/toast'
import { pb } from '~/util/path-builder'

const projectView = ({ project }: { project: string }) =>
apiq('projectView', { path: { project } })
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing that these functions from selectors to query args are totally generic, likely to be used in multiple places, and could even be generated. In the meantime I'll try collecting some in a file.

@david-crespo
Copy link
Collaborator Author

I'm happy with invalidateEndpoint. I like the direction, so I'm going to merge. Unfortunately the codebase will continue to be a mix of new and old until everything is converted, but I think it's tolerable.

@david-crespo david-crespo enabled auto-merge (squash) December 6, 2024 18:54
@david-crespo david-crespo disabled auto-merge December 6, 2024 19:01
@david-crespo david-crespo force-pushed the convert-query-client-example branch from 127a6b3 to 604ecf5 Compare December 6, 2024 19:05
@david-crespo david-crespo enabled auto-merge (squash) December 6, 2024 19:07
@david-crespo david-crespo merged commit 01a0ac9 into main Dec 6, 2024
8 checks passed
@david-crespo david-crespo deleted the convert-query-client-example branch December 6, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant