Skip to content

Fix refetchInterval infinite loop #404

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 1, 2020
Merged

Fix refetchInterval infinite loop #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b2ca26
use queryRef to avoid infinite loop in refetch interval effect
cherniavskii Apr 25, 2020
63a6c6f
fix original config being mutated by config from another hook instance
cherniavskii Apr 30, 2020
10b6e52
store refetchIntervalId and fix refetchInterval comparison
cherniavskii Apr 30, 2020
cc9fd67
clear query.currentRefetchInterval on cleanup
cherniavskii Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/queryCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export function makeQueryCache() {

if (query) {
Object.assign(query, { queryVariables, queryFn })
Object.assign(query.config, config)
query.config = { ...query.config, ...config }
Copy link
Contributor Author

@cherniavskii cherniavskii Apr 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tannerlinsley

Object.assign was causing a bug. Consider this situation:

  1. first instance of useQuery creates a new query and sets query.config pointer to config object
  2. second instance of useQuery with different config object takes query from cache
  3. Object.assign mutates first instance query.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

} else {
query = makeQuery({
queryKey,
Expand Down
22 changes: 11 additions & 11 deletions src/useBaseQuery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,28 +89,28 @@ 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)
(!query.currentRefetchInterval ||
// shorter interval should override previous one
config.refetchInterval < query.currentRefetchInterval)
) {
clearInterval(query.refetchInterval)
query.refetchInterval = setInterval(() => {
query.currentRefetchInterval = config.refetchInterval
clearInterval(query.refetchIntervalId)
query.refetchIntervalId = setInterval(() => {
if (isDocumentVisible() || config.refetchIntervalInBackground) {
refetch().catch(Console.error)
}
}, config.refetchInterval)

return () => {
clearInterval(query.refetchInterval)
delete query.refetchInterval
clearInterval(query.refetchIntervalId)
delete query.refetchIntervalId
delete query.currentRefetchInterval
}
}
}, [
config.refetchInterval,
config.refetchIntervalInBackground,
query.refetchInterval,
refetch,
])
}, [config.refetchInterval, config.refetchIntervalInBackground, refetch])

return {
...query.state,
Expand Down