-
Notifications
You must be signed in to change notification settings - Fork 887
chore(site): remove users and pagination services #9932
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
Conversation
|
||
export const users = (req: UsersRequest) => { | ||
return { | ||
queryKey: ["users", req], |
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.
should we really be using req
as a key here? not req.id
or something?
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.
Req is basically the page
, offset
and filter
params so in this case I think makes sense to use them together. Reference: https://tanstack.com/query/v4/docs/react/guides/query-keys#query-keys-are-hashed-deterministically
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.
Yeah, it's not always needed, but I've seen the pattern where the payload for the query is treated as the last argument of the query key. It also makes it possible for the query function to access it via the queryKey
property on its options argument, even if you extract the query function outside the hook
<> | ||
<MenuItem | ||
component="a" | ||
href={learnMoreLink2} | ||
target="_blank" | ||
sx={{ fontSize: 13, fontWeight: 500 }} | ||
onClick={() => { | ||
setIsOpen(false); | ||
}} | ||
> | ||
<OpenInNewOutlined sx={{ fontSize: "14px !important" }} /> | ||
{learnMoreLabel2} | ||
</MenuItem> | ||
</> |
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.
nice bit of cleanup!
rolesQuery.isLoading || | ||
authMethods.isLoading; | ||
usersQuery.isLoading || rolesQuery.isLoading || authMethodsQuery.isLoading; | ||
// Suspend |
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.
these comments don't really add much value, and feel like the kind that are prone to eventual drift. maybe just separate each group with a blank line?
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.
Ok, I personally like to use comments to separate things sometimes 😁 going to remove them and add blank lines instead
…r-users-service
…coder into bq/refactor-users-service
return { | ||
mutationFn: API.createUser, | ||
onSuccess: async () => { |
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.
On line 24, we don't have a closure like we do on line 8. Do you know why? Referencing this convo in Slack the other day.
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 because one is a query and another one a mutation so when it is a mutation, you can pass the args on the call like createUser.mutateAsync(...)
const { data: userLoginType } = useQuery({ | ||
queryKey: ["loginType"], | ||
queryFn: getUserLoginType, | ||
}); | ||
const singleSignOnSection = useSingleSignOnSection(); | ||
|
||
if (!authMethods || !userLoginType) { | ||
if (!authMethodsQuery.data || !userLoginType) { |
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.
Do we handle error state? Does data
always return (empty) if the request errors?
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.
yeah, error values are always exposed through error
and isError
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 ya in hell xservice
🚪
…r-users-service
Related to #9598