[UI v2] poc: Re-organize variable queries and mutations using queryOptions factory pattern #16155
Conversation
devinvillarosa
added
development
devinvillarosa
requested review from
aaazzam,
cicdw,
desertaxle and
zzstoatzz
as code owners
devinvillarosa
force-pushed
the
query-options-variables
branch
3 times, most recently
from
4fa2cf9
to
6a82e26
Compare
…tions factory pattern
devinvillarosa
force-pushed
the
query-options-variables
branch
from
6a82e26
to
eb0e61e
Compare
ui-v2/src/hooks/variables.ts
Outdated
| placeholderData: keepPreviousData, | ||
| }), | ||
| counts: () => [...variableQueries.all(), "count"] as const, | ||
| count: (variables?: Variables) => { |
There was a problem hiding this comment.
updated the argument here so that it is specifically looking for a variables type to be passed.
Changes are reflected down in L143 and L191
There was a problem hiding this comment.
Thanks for taking the time to think through organization structures for this module! Right now, I don't think that a query factory like the one shown in the linked blog post is what I would choose. I would prefer to have a query key factories and query options factories separate but closely located in the same module, but I'm happy to discuss further!
ui-v2/src/hooks/variables.ts
Outdated
| @@ -11,82 +11,91 @@ import { | |||
|
|
|||
| type UseVariablesOptions = | |||
| components["schemas"]["Body_read_variables_variables_filter_post"]; | |||
| type Variables = UseVariablesOptions["variables"]; | |||
There was a problem hiding this comment.
We can use the existing schema from the type generation to avoid coupling between the types.
| type Variables = UseVariablesOptions["variables"]; | |
| type VariablesFilter = components["schemas"]["VariableFilter"]; |
ui-v2/src/hooks/variables.ts
Outdated
| return queryOptions({ | ||
| queryKey: [...variableQueries.counts(), body] as const, | ||
| queryFn: () => fetchVariableCountRequest(body), | ||
| staleTime: 1000, | ||
| placeholderData: keepPreviousData, | ||
| }); |
There was a problem hiding this comment.
We want to ensure that we aren't including the body in the query key variables is undefined to avoid making duplicate variable count calls when the variables page first mounts.
| staleTime: 1000, | ||
| placeholderData: keepPreviousData, | ||
| }); | ||
| const variableQueries = { |
There was a problem hiding this comment.
It strikes me as odd that some of these functions return a query key while others return a full queryOptions object. The heterogenous return types make me think these should be grouped differently.
There was a problem hiding this comment.
Ya, might need to go back to the drawing board on this one.
It is a bit strange that the objects are inconsistent, but I do agree with the blog on how the Dev experience much better when they're coupled together. 🤔
np! it's just a proof of concept. Not too attached to any decisions. |
What does this PR do and why?
This PR expands on the current variables query key factory pattern, but moves it using
queryOptions. Additionally, while usingqueryOptions, this movesqueryKeysto be associated withqueryFn.Blog for a good read on why we should associate the two together
Notable changes:
@tanstack/eslint-plugin-queryas a dev dependency. This eslint rule checks if variables passed in thequeryFnis also passed as aqueryKeyqueryFnandmutationFninto its own function wrapper. Purpose is to separate concerns from APIs and queries 🧅. Additionally most @TanStack docs follow a similar patternBefore
After
Checklist
<link to issue>"mint.json.Related to #15512