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.

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(query-core): allow multiple roots to share same QueryClient #4636

Merged
merged 4 commits into from Jan 4, 2023

Conversation

appden
Copy link
Contributor

@appden appden commented Dec 13, 2022

If two roots shared the QueryClient instance, then it would never unsubscribe from online/focus manager listeners and would also forever receive duplicate events because the unsubscriber functions would get "forgotten".

These issues would be especially noticeable on React Native apps where multiple roots that share the same QueryClient are common.

If two roots shared the `QueryClient` instance, then it would never unsubscribe from online/focus manager listeners and would also forever receive duplicate events because the unsubscriber functions would get "forgotten".

These issues would be especially noticeable on React Native apps where multiple roots that share the same `QueryClient` are common.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 13, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 27ad7dc:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-vue-basic Configuration

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2022

Codecov Report

Base: 96.36% // Head: 92.40% // Decreases project coverage by -3.95% ⚠️

Coverage data is based on head (f00b8e7) compared to base (eab6e2c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4636      +/-   ##
==========================================
- Coverage   96.36%   92.40%   -3.96%     
==========================================
  Files          45       89      +44     
  Lines        2281     3765    +1484     
  Branches      640      984     +344     
==========================================
+ Hits         2198     3479    +1281     
- Misses         80      270     +190     
- Partials        3       16      +13     
Impacted Files Coverage Δ
src/core/queryClient.ts
src/devtools/utils.ts
src/core/subscribable.ts
src/react/useInfiniteQuery.ts
src/core/logger.ts
src/core/query.ts
src/devtools/Logo.tsx
src/core/queryCache.ts
src/devtools/styledComponents.ts
src/core/retryer.ts
... and 124 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 85 to +88
mount(): void {
this.mountCount++
if (this.mountCount !== 1) return

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.

@TkDodo TkDodo merged commit 05a61b1 into TanStack:main Jan 4, 2023
@appden appden deleted the fix-multiple-roots branch January 4, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants