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

Let react-query manage observer count #50

Merged
merged 3 commits into from
Jul 12, 2022

Conversation

0xR
Copy link

@0xR 0xR commented Jul 10, 2022

Fixes #25 (comment)

React-query-firebase was managing it's own observerCount, but it was incorrect. React-query already does this and is was correct.

When there are no observers firestore is unsubscribed. If there are observers it is added again.

Also improved the readability of the code a bit by using more variables / helper functions.

Verified it is working in my application that was having issues.

I had some trouble running the tests.

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2022

CLA assistant check
All committers have signed the CLA.

@cabljac
Copy link
Contributor

cabljac commented Jul 12, 2022

this lgtm, I'll see if i can get tests to pass :) Thanks!!

@cabljac cabljac merged commit ddc98d0 into invertase:dev-release Jul 12, 2022
@cabljac
Copy link
Contributor

cabljac commented Jul 12, 2022

hmm i'm not sure this is working properly when a key is changed? At least that seems to be why this first test is failing

@0xR
Copy link
Author

0xR commented Jul 13, 2022

It should work with changing keys since the hook will run with the new key and notice there is no queryCacheUnsubscribe with that subscriptionHash or a firestoreUnsubscribe. Therefore it will subscribe to both the queryCache and firestore itself. The old key will get 0 subscribers and be cleaned up.

But I'll have a look at the tests this weekend. I'm able to run them now and see the test failures.

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.

3 participants