-
-
Notifications
You must be signed in to change notification settings - Fork 491
feat: enhance React Query integration with per-type query key support #2361
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
base: master
Are you sure you want to change the base?
Conversation
packages/query/src/index.ts
Outdated
${ | ||
!queryKeyMutator | ||
? queries | ||
.filter( |
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.
reject all non unique queryKeyFnName for prevent generate duplication functions
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.
Does this mean that the issue was already there?
Or has this change meant that there is now a possibility of duplication?
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.
After the discussion, if we define this processing, insert a variable definition such as uniqueQueryKeys
to make the nesting shallowe.
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.
Does this mean that the issue was already there?
No, this is an introduced problem. This happens because each query type has its queryKeyFnName
For example QueryType.QUERY and QueryType.SUSPENSE_QUERY has same queryKeyFnName
Probably we need drop queryKeyFnName from queries
array and use somewhat like
const queryKeyFnName = query.type === QueryType.INFINITE || query.type === QueryType.SUSPENSE_INFINITE ? camel(`get-${operationName}-infinite-query-key`) : camel(`get-${operationName}-query-key`) ```
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.
Unfortunately, the simple solution didn't work.
We need an array of keys: one for regular queries, the other for infinite ones.
Therefore, we have to iterate through the array of queries to collect the required ones.
If we stick with the ternary solution I suggested above, we'll end up with duplicate keys.
Perhaps I'm approaching the problem incorrectly and there's a simpler way. If so, I'd appreciate any hints.
77f8f56
to
051e3b2
Compare
options: query?.options, | ||
type: QueryType.SUSPENSE_INFINITE, | ||
queryParam: query?.useInfiniteQueryParam, | ||
queryKeyFnName: camel(`get-${operationName}-infinite-query-key`), |
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.
now each query type has own queryKeyFnName
38cd852
to
24fc7d6
Compare
this LGTM but i would like @soartec-lab to review also. |
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.
Thanks for the changes. I added some comments.
packages/query/src/utils.ts
Outdated
|
||
// Convert "param: Type" to "param?: Type" for queryKey functions | ||
// to enable cache invalidation without type assertion | ||
export const makeParamsOptional = (params: string) => { |
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.
Currently, we are not strictly extracting functions into utils.
There are also benefits to having functions exposed in the files you use, closer to the processing they are used in.
What was the reason for moving them here now?
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.
I felt that this was a rather isolated and generic function and it didn't belong in the index file.
After all, the index file is too big and hard to understand.
packages/query/src/index.ts
Outdated
${[ | ||
routeString, | ||
query.type === QueryType.INFINITE || | ||
query.type === QueryType.SUSPENSE_INFINITE | ||
? `'infinite'` | ||
: '', | ||
queryParams ? '...(params ? [params]: [])' : '', | ||
body.implementation, | ||
] | ||
.filter((x) => !!x) | ||
.join(', ')} | ||
] as const; |
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.
Wouldn't it be enough to just prepend the string to the array for infinate
?
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.
Wht not, i agree
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.
Great, then let's just add a prefix for a more concise fix.
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.
Added the prefix as you suggested
packages/query/src/index.ts
Outdated
${ | ||
!queryKeyMutator | ||
? queries | ||
.filter( |
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.
Does this mean that the issue was already there?
Or has this change meant that there is now a possibility of duplication?
packages/query/src/index.ts
Outdated
${ | ||
!queryKeyMutator | ||
? queries | ||
.filter( |
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.
After the discussion, if we define this processing, insert a variable definition such as uniqueQueryKeys
to make the nesting shallowe.
packages/query/src/index.ts
Outdated
return '{ signal }'; | ||
}; | ||
|
||
const generateQueryKeyImplementation = ({ |
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.
By extracting the function, the distance between the declaration and the place where it is actually used has increased. This increases the burden of understanding the overall processing, so consider reverting it if there is no significant benefit.
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.
Yes, I understand what you're talking about. However, I'll put in my two cents:
I thought that in this way I was actually increasing the clarity of the code and its maintainability
generateQueryImplementation is already a very large function
Now the generator functions are divided by type of responsibility:
@Armanio So I'd like to separate feature additions and refactorings. |
Of course! Look a great plan. Give me little more time 🤝 |
24fc7d6
to
027d6d4
Compare
…support - Generate unique query keys for infinite vs regular queries
027d6d4
to
2ffac79
Compare
@soartec-lab |
Fix #2359
Status
READY
Description
PS: I'm not sure if this edit requires a doc edit, if not, please correct me