Skip to content
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

Jump to bottom

fix(query-core): allow multiple roots to share same QueryClient #4636

Merged
merged 4 commits into from Jan 4, 2023
Merged

fix(query-core): allow multiple roots to share same QueryClient #4636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac7cbd3
fix(query-core): allow multiple roots to share same QueryClient
appden Dec 13, 2022
f00b8e7
Merge branch 'main' into fix-multiple-roots
TkDodo Dec 18, 2022
a5d7003
test(QueryClient): Add test for multiple mounts, single unmount
appden Dec 20, 2022
27ad7dc
Merge branch 'main' into fix-multiple-roots
TkDodo Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/query-core/src/queryClient.ts
View file
Open in desktop
Expand Up @@ -62,6 +62,7 @@ export class QueryClient {
private defaultOptions: DefaultOptions
private queryDefaults: QueryDefaults[]
private mutationDefaults: MutationDefaults[]
private mountCount: number
private unsubscribeFocus?: () => void
private unsubscribeOnline?: () => void

Expand All @@ -72,6 +73,7 @@ export class QueryClient {
this.defaultOptions = config.defaultOptions || {}
this.queryDefaults = []
this.mutationDefaults = []
this.mountCount = 0

if (process.env.NODE_ENV !== 'production' && config.logger) {
this.logger.error(
Expand All @@ -81,6 +83,9 @@ export class QueryClient {
}

mount(): void {
this.mountCount++
if (this.mountCount !== 1) return

Comment on lines 85 to +88
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we alternatively just call this.unmount() at the beginning of the mount function here? Basically saying: "If you mount a second time, we unmount the first instance", as opposed to: "If you mount a second time, we don't do anything because you're already mounted" ....

I've tried your test with this approach and it passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look at this! You're right, the test perhaps could be better, but that approach would break if we mounted two roots that share a client, then unmounted just one. In that circumstance, we'd have unsubscribed even though there is still one root mounted, hence the need for a count. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TkDodo I just pushed up an additional test to cover the case I described above.

this.unsubscribeFocus = focusManager.subscribe(() => {
if (focusManager.isFocused()) {
this.resumePausedMutations()
Expand All @@ -96,8 +101,14 @@ export class QueryClient {
}

unmount(): void {
this.mountCount--
if (this.mountCount !== 0) return

this.unsubscribeFocus?.()
this.unsubscribeFocus = undefined

this.unsubscribeOnline?.()
this.unsubscribeOnline = undefined
}

isFetching(filters?: QueryFilters): number
Expand Down
36 changes: 36 additions & 0 deletions packages/query-core/src/tests/queryClient.test.tsx
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ describe('queryClient', () => {

afterEach(() => {
queryClient.clear()
queryClient.unmount()
})

describe('defaultOptions', () => {
Expand Down Expand Up @@ -1466,6 +1467,41 @@ describe('queryClient', () => {
mutationCacheResumePausedMutationsSpy.mockRestore()
onlineManager.setOnline(undefined)
})

test('should not notify queryCache and mutationCache after multiple mounts/unmounts', async () => {
const testClient = createQueryClient()
testClient.mount()
testClient.mount()
testClient.unmount()
testClient.unmount()

const queryCacheOnFocusSpy = jest.spyOn(
testClient.getQueryCache(),
'onFocus',
)
const queryCacheOnOnlineSpy = jest.spyOn(
testClient.getQueryCache(),
'onOnline',
)
const mutationCacheResumePausedMutationsSpy = jest.spyOn(
testClient.getMutationCache(),
'resumePausedMutations',
)

onlineManager.setOnline(true)
expect(queryCacheOnOnlineSpy).not.toHaveBeenCalled()
expect(mutationCacheResumePausedMutationsSpy).not.toHaveBeenCalled()

focusManager.setFocused(true)
expect(queryCacheOnFocusSpy).not.toHaveBeenCalled()
expect(mutationCacheResumePausedMutationsSpy).not.toHaveBeenCalled()

queryCacheOnFocusSpy.mockRestore()
queryCacheOnOnlineSpy.mockRestore()
mutationCacheResumePausedMutationsSpy.mockRestore()
focusManager.setFocused(undefined)
onlineManager.setOnline(undefined)
})
})

describe('setMutationDefaults', () => {
Expand Down