-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix refetchInterval infinite loop #404
Fix refetchInterval infinite loop #404
Conversation
src/useBaseQuery.js
Outdated
@@ -89,6 +89,7 @@ export function useBaseQuery(queryKey, queryVariables, queryFn, config = {}) { | |||
|
|||
// Handle refetch interval | |||
React.useEffect(() => { | |||
const query = queryRef.current | |||
if ( | |||
config.refetchInterval && | |||
(!query.refetchInterval || config.refetchInterval < query.refetchInterval) |
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 think query.refetchInterval
name is confusing and should be renamed to query.refetchIntervalId
, since it actually stores interval ID.
Then config.refetchInterval < query.refetchInterval
comparison doesn't make sense to me.
As well as !query.refetchInterval
check, since query.refetchInterval
will be always undefined
due to cleaning function returned by useEffect.
@tannerlinsley thoughts?
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, the idea was that if a user passed different intervals for the same query but on different instances, then the shortest interval time would override the longer one.
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 work as you described now!
Wanted to add a unit test fot this case, but it involves dealing with timers, which can be tricky and I did't want to invest my time into this right now. |
@@ -110,7 +110,7 @@ export function makeQueryCache() { | |||
|
|||
if (query) { | |||
Object.assign(query, { queryVariables, queryFn }) | |||
Object.assign(query.config, config) | |||
query.config = { ...query.config, ...config } |
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.
Object.assign
was causing a bug. Consider this situation:
- first instance of useQuery creates a new query and sets query.config pointer to config object
- second instance of useQuery with different config object takes query from cache
Object.assign
mutates first instancequery.config
object
Now as I think about it, maybe we should copy query config in the first step instead of referencing it?
Either way, it should be good now.
But that was super tough to debug :D
Yes!! This was a total doozy |
Use queryRef to avoid infinite loop in refetch interval effect.
Fixes #399