-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
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.
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:
|
Codecov ReportBase: 96.36% // Head: 92.40% // Decreases project coverage by
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 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. |
mount(): void { | ||
this.mountCount++ | ||
if (this.mountCount !== 1) return | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😀
There was a problem hiding this comment.
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.
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.