-
Notifications
You must be signed in to change notification settings - Fork 22
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
chore: Migrate useSelfHostedUserList to TS Query V5 #3575
chore: Migrate useSelfHostedUserList to TS Query V5 #3575
Conversation
Bundle ReportChanges will increase total bundle size by 431 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will increase total bundle size by 431 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3575 +/- ##
=======================================
Coverage 98.99% 98.99%
=======================================
Files 810 810
Lines 14567 14569 +2
Branches 4157 4149 -8
=======================================
+ Hits 14420 14422 +2
Misses 140 140
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3575 +/- ##
=======================================
Coverage 98.99% 98.99%
=======================================
Files 810 810
Lines 14567 14569 +2
Branches 4157 4149 -8
=======================================
+ Hits 14420 14422 +2
Misses 140 140
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3575 +/- ##
=======================================
Coverage 98.99% 98.99%
=======================================
Files 810 810
Lines 14567 14569 +2
Branches 4150 4149 -1
=======================================
+ Hits 14420 14422 +2
Misses 140 140
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
) | ||
|
||
return { | ||
...rest, |
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.
any chance we can avoid using ...rest here? Do we really need everything coming back from this query call?
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.
sure, we can remove it
isAdmin?: boolean | ||
} | ||
|
||
const useQueryMembersList = (params: Params) => { |
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.
Im also kinda curious why we pulled the memo out of the query and into this new query in the component itself
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.
Beforehand the service query was a custom hook so we're okay to use a useMemo
in there, whereas now with the queryOptions
factory function it is no longer a custom hook, so we're not able to use a hook inside of the function.
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.
makes sense!
return parsedData.data | ||
}) | ||
}, | ||
suspense: false, | ||
initialPageParam: '1', |
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.
is this not defaulted to 1 already?
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.
It is not, you have to explicitly define it in V5
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: initialPageParam
(2nd required option) https://tanstack.com/query/latest/docs/framework/react/reference/useInfiniteQuery
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.
1 small question, approved otherwise
522581a
to
7636bf9
Compare
Description
This PR migrates the
useSelfHostedUserList
to the infiniteQueryOptions setup from TS Query V5 toSelfHostedUserListQueryOpts
Ticket: codecov/engineering-team#2961
Notable Changes
useSelfHostedUserList
toSelfHostedUserListQueryOpts
MemberTable