-
Notifications
You must be signed in to change notification settings - Fork 0
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
Overhaul caching / Add React-Query #35
Conversation
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.
Seems to work well here!
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.
LGTM, have a comment about default stale time
export const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
gcTime: 1000 * 60 * 60 * 24, // 24 hours |
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 set a staleTime default to non-zero to reduce extra calls from our provider library? (we use 10 seconds in the website)
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.
My main concern about caching is about getting updated provider connections from our APIs; if a user connects an integration and goes straight to the popup, I'd expect they would be able to grant permissions right away.
That being said I think a blanket rule of 10 seconds should be short enough 👍
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.
The only hook that uses the provider library currently actually already has a stale time set to 10 minutes (with immediate invalidation if the user clicks on one of the links, since there's a reasonable chance that the user may perform an action on the PR).
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 can still add a global 10 second stale time though
https://gitkraken.atlassian.net/browse/GKCS-5568