Skip to content

[WIP] New useQueryState Hook #5

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
fix: useQueryState impl
the value of the ref can get out of sync if we only write the ref in the subscribe function
  • Loading branch information
TkDodo committed Jan 24, 2024
commit 4f000317b618878aaabd91d21695f74ffcd0808f
22 changes: 4 additions & 18 deletions packages/react-query/src/__tests__/useIsFetching.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,11 @@ describe('useIsFetching', () => {
const queryClient = createQueryClient()
const key = queryKey()

let resolve!: () => void
const promise = new Promise<void>((_resolve) => {
resolve = _resolve
})

function Page() {
useQuery({
queryKey: key,
queryFn: async () => {
await promise
await sleep(100)
return 'test'
},
})
Expand All @@ -202,35 +197,27 @@ describe('useIsFetching', () => {
const rendered = renderWithClient(queryClient, <Page />)

await rendered.findByText('isFetching: 1')

resolve()

await rendered.findByText('isFetching: 0')
})

it('should use provided custom queryClient', async () => {
const queryClient = createQueryClient()
const key = queryKey()

let resolve!: () => void
const promise = new Promise<void>((_resolve) => {
resolve = _resolve
})

function Page() {
const isFetching = useIsFetching({}, queryClient)

useQuery(
{
queryKey: key,
queryFn: async () => {
await promise
await sleep(10)
return 'test'
},
},
queryClient,
)

const isFetching = useIsFetching({}, queryClient)

return (
<div>
<div>isFetching: {isFetching}</div>
Expand All @@ -241,6 +228,5 @@ describe('useIsFetching', () => {
const rendered = render(<Page></Page>)

await waitFor(() => rendered.getByText('isFetching: 1'))
resolve()
})
})
29 changes: 13 additions & 16 deletions packages/react-query/src/useQueryState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ function getResult<TResult = QueryState>(
.findAll(options.filters)
.map(
(query): TResult =>
(options.select
? options.select(
query as Query<unknown, DefaultError, unknown, QueryKey>,
)
: query.state) as TResult,
(options.select ? options.select(query) : query.state) as TResult,
)
}

Expand All @@ -52,19 +48,20 @@ export function useQueryState<TResult = QueryState>(
return React.useSyncExternalStore(
React.useCallback(
(onStoreChange) =>
queryCache.subscribe(() => {
const nextResult = replaceEqualDeep(
result.current,
getResult(queryCache, optionsRef.current),
)
if (result.current !== nextResult) {
result.current = nextResult
notifyManager.schedule(onStoreChange)
}
}),
queryCache.subscribe(notifyManager.batchCalls(onStoreChange)),
Copy link

Choose a reason for hiding this comment

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

Memory Management: While the current implementation works correctly in most cases, it's a best practice to explicitly return the unsubscribe function from the subscribe callback in useSyncExternalStore. This ensures proper cleanup when components unmount, preventing potential memory leaks in complex applications or during rapid component mounting/unmounting cycles.

Suggested change
queryCache.subscribe(notifyManager.batchCalls(onStoreChange)),
(onStoreChange) => {
const unsubscribe = queryCache.subscribe(notifyManager.batchCalls(onStoreChange))
return unsubscribe // Explicitly return unsubscribe function for cleanup
},

[queryCache],
),
() => result.current,
() => {
const nextResult = replaceEqualDeep(
result.current,
getResult(queryCache, optionsRef.current),
)
if (result.current !== nextResult) {
result.current = nextResult
}

return result.current
},
() => result.current,
)!
}