Skip to content

Commit

Permalink
fix(query-core): make sure accessing one property on a query result i…
Browse files Browse the repository at this point in the history
…n useQueries tracks properties across all observers (#7014)

* fix(query-core): make sure accessing one property on a query result in useQueries tracks properties across all observers

In useQueries, property tracking is still performed on each individual observer. However, not all invocations will access the property on all observers. For example:

```
results.some((result) => result.isLoading)
```

will only track the `isLoading` property of the first query in the array if `isLoading` is true, because the loop will eagerly abort, and thus the property isn't accessed at all on other observer, which can lead to missing re-renders.

This fix adds a callback to `trackProperties`, which will be invoked when we start to track a property on an observer. Then, in useQueries, we can use this callback to trigger the accessor manually for all other observers.

That makes sure that if a property like `isLoading` is accessed on one observer, it is tracked for all of them.

* refactor: expose trackProp function
  • Loading branch information
TkDodo authored Mar 3, 2024
1 parent b311a0a commit 5e64873
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 2 deletions.
7 changes: 6 additions & 1 deletion packages/query-core/src/queriesObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,12 @@ export class QueriesObserver<
return matches.map((match, index) => {
const observerResult = result[index]!
return !match.defaultedQueryOptions.notifyOnChangeProps
? match.observer.trackResult(observerResult)
? match.observer.trackResult(observerResult, (accessedProp) => {
// track property on all observers to ensure proper (synchronized) tracking (#7000)
matches.forEach((m) => {
m.observer.trackProp(accessedProp)
})
})
: observerResult
})
},
Expand Down
8 changes: 7 additions & 1 deletion packages/query-core/src/queryObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ export class QueryObserver<

trackResult(
result: QueryObserverResult<TData, TError>,
onPropTracked?: (key: keyof QueryObserverResult) => void,
): QueryObserverResult<TData, TError> {
const trackedResult = {} as QueryObserverResult<TData, TError>

Expand All @@ -255,7 +256,8 @@ export class QueryObserver<
configurable: false,
enumerable: true,
get: () => {
this.#trackedProps.add(key as keyof QueryObserverResult)
this.trackProp(key as keyof QueryObserverResult)
onPropTracked?.(key as keyof QueryObserverResult)
return result[key as keyof QueryObserverResult]
},
})
Expand All @@ -264,6 +266,10 @@ export class QueryObserver<
return trackedResult
}

trackProp(key: keyof QueryObserverResult) {
this.#trackedProps.add(key)
}

getCurrentQuery(): Query<TQueryFnData, TError, TQueryData, TQueryKey> {
return this.#currentQuery
}
Expand Down
50 changes: 50 additions & 0 deletions packages/react-query/src/__tests__/useQueries.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,56 @@ describe('useQueries', () => {
expect(results.length).toBe(length)
})

it('should synchronously track properties of all observer even if a property (isLoading) is only accessed on one observer (#7000)', async () => {
const key = queryKey()
const ids = [1, 2]

function Page() {
const { isLoading } = useQueries({
queries: ids.map((id) => ({
queryKey: [key, id],
queryFn: () => {
return new Promise<{
id: number
title: string
}>((resolve, reject) => {
if (id === 2) {
setTimeout(() => {
reject(new Error('FAILURE'))
}, 10)
}
setTimeout(() => {
resolve({ id, title: `Post ${id}` })
}, 10)
})
},
retry: false,
})),
combine: (results) => {
// this tracks data on all observers
void results.forEach((result) => result.data)
return {
// .some aborts early, so `isLoading` might not be accessed (and thus tracked) on all observers
// leading to missing re-renders
isLoading: results.some((result) => result.isLoading),
}
},
})

return (
<div>
<p>Loading Status: {isLoading ? 'Loading...' : 'Loaded'}</p>
</div>
)
}

const rendered = renderWithClient(queryClient, <Page />)

await waitFor(() => rendered.getByText('Loading Status: Loading...'))

await waitFor(() => rendered.getByText('Loading Status: Loaded'))
})

it('should not have stale closures with combine (#6648)', async () => {
const key = queryKey()

Expand Down

0 comments on commit 5e64873

Please sign in to comment.