feat: rework realtime subscription hooks

[Ehesp]

Fixes #25

This PR reworks hooks that depend on realtime subscriptions. Currently, if a hook declares a realtime subscription (via the subscribe: true option or by default (e.g. auth hooks)), a staleTime of Infinity is being set by default. If other hooks with the same QueryKey are created, the existing subscription will be used (which is expected).

The issue is, "Infinity" data is never considered stale for the remainder of the applications lifecycle. So assuming there is always an active subscription then this doesn't cause any issues (since the subscription always updates the cache). However, if all active hooks are unmounted, and remounted, the actual queryFn won't be executed again (since it'll just return the data within the query cache). However since all hooks have actually unmounted, there is no active subscription thus the data will never be updated again.

This PR basically reworks the effected hooks by;

1. Tracking the query key "mounts"
2. If the is existing query keys, then it'll simple return the cached data
3. If there is no existing query keys, it'll create a new subscription & store that reference
4. Each time a hook unmounts, the query key is subtracted from the active counter. If it's zero (no more hooks active), it'll unsubscribe.

### TODOs

• Auth
• Firestore
• RTDB
• Add tests to test these conditions
• Extract / publish the utils package (since it causes weird building if it's directly import via rollup).
• Ensure all files have copyright headers

[docs-page]

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/react-query-firebase~35

Documentation is deployed and generated using docs.page.

[atanaskanchev]

Hello, is there anything we can help to push this PR over the line?

[estebanrao]

Happy to help as well, this fix is crucial for an implementation in prod :)

[atanaskanchev]

Happy to help as well, this fix is crucial for an implementation in prod :)

I'm also stuck and can't go to production without this fix. I've had great hopes for this lib as it was used by Google showcasing Firebase 9, sadly it seems the repo is abandoned, I'll start looking for alternatives.

[hchor]

Has anyone else tested this? It looks like the owner says it should work but is concerned about edge cases.

[Ehesp]

I'm going to try and get some resource from the company assigned to this soon, apologies it's been really busy.

[hchor]

I'm going to try and get some resource from the company assigned to this soon, apologies it's been really busy.

This can't be stated enough but the community greatly appreciates your work!

[cabljac]

(just an update)

Currently working on this, experiencing some strange behaviour with the hooks in a test, onSuccess is called more times than i'd expect when rerendering.

[cabljac]

OK actually made some progress on this, curious what people think. I'm using a library called react-query-subscription which seems to work fairly well. Interested in any opinions here. A work in progress example:

import { QueryKey } from "react-query";
import { Auth, IdTokenResult, AuthError } from "firebase/auth";
import { Observable } from "rxjs";
import {
  useSubscription,
  UseSubscriptionOptions,
} from "react-query-subscription";
import { UseSubscriptionResult } from "react-query-subscription/types/use-subscription";

export function idTokenFromAuth(
  auth: Auth,
  options?: {
    forceRefresh?: boolean;
  }
): Observable<IdTokenResult | null> {
  return new Observable<IdTokenResult | null>(function subscribe(subscriber) {
    const unsubscribe = auth.onIdTokenChanged(async (user) => {
      let token: IdTokenResult | null = null;

      if (user) {
        token = await user.getIdTokenResult(options?.forceRefresh);
      }

      subscriber.next(token);
    });
    subscriber.add(unsubscribe);
  });
}

export function useAuthIdToken<R = IdTokenResult | null>(
  key: QueryKey,
  auth: Auth,
  options?: {
    forceRefresh?: boolean;
  },
  useSubscriptionOptions?: Omit<
    UseSubscriptionOptions<IdTokenResult | null, AuthError, R>,
    "queryFn"
  >
): UseSubscriptionResult<R | AuthError> {
  return useSubscription(key, () => idTokenFromAuth(auth, options), {
    ...useSubscriptionOptions,
  });
}

[cabljac]

tests still need fixing etc, and will rewrite the rest of the subscriptions like this.

[estebanrao]

Is adding react-query-subscription really necessary? A dependency like this can cause issues in the future is no one maintains that package

[cabljac]

Is adding react-query-subscription really necessary? A dependency like this can cause issues in the future is no one maintains that package

This is a fair point to be honest, it just seemed like they handled subscriptions quite nicely. @Ehesp What do you think?

[cabljac]

Yeah react-query-subscription seems to contain a few extra pieces we don't need. I think the solution to this bug is there, so i'll try and do something similar.

[estebanrao]

@cabljac great! let us know when we can jump in and test! :)

[cabljac]

hm seems a bit trickier to implement than initially hoped.

[estebanrao]

Even replicating the parts you need from react-query-subscription?

[cabljac]

Made some headway! essentially tweaking @Ehesp's solution a bit, but taking some ideas from react-query-subscription too. It seems to be passing tests.

[codecov]

Codecov Report

Merging #35 (b25bd42) into main (dc470cc) will decrease coverage by 2.83%.
The diff coverage is 75.24%.

❗ Current head b25bd42 differs from pull request most recent head b72191c. Consider uploading reports for the commit b72191c to get more accurate results

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   62.57%   59.73%   -2.84%     
==========================================
  Files          12       22      +10     
  Lines         489      457      -32     
  Branches       60       44      -16     
==========================================
- Hits          306      273      -33     
- Misses        183      184       +1     

Impacted Files	Coverage Δ
packages/auth/src/mutations.ts	0.00% <ø> (ø)
...ages/auth/src/useAuthFetchSignInMethodsForEmail.ts	0.00% <0.00%> (ø)
packages/auth/src/useAuthGetRedirectResult.ts	0.00% <0.00%> (ø)
packages/database/src/useDatabaseMutation.ts	100.00% <ø> (ø)
packages/firestore/src/mutations.ts	100.00% <ø> (ø)
packages/firestore/src/namedQuery.ts	9.09% <9.09%> (ø)
packages/firestore/src/useFirestoreQueryData.ts	48.00% <48.00%> (ø)
packages/firestore/src/useFirestoreDocumentData.ts	52.63% <52.63%> (ø)
packages/firestore/src/index.ts	78.26% <78.26%> (ø)
packages/utils/src/useSubscription.ts	91.83% <91.83%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc470cc...b72191c. Read the comment docs.

[cabljac]

I think I've refactored all the subscription hooks now:

• useAuthUser
• useAuthIdToken
• useFirestoreDocument
• useFirestoreDocumentData
• useFirestoreQuery
• useFirestoreQueryData
• useDatabaseSnapshot
• useDatabaseValue

If anyone notices there's one missing in this list let me know :). I've also added tests for the issue, which seem to pass.
I need to ensure all files have copyright headers, and give things another look/ add a couple of code comments to the new useSubscription utility hook.

I have a feeling there's also some work on the error handling to do, and possibly some tests there.

ALSO so i don't forget later, two points that i'll put in separate issues:

• apparently react-query 4 does not call onSuccess when queryClient.setQueryData is called. This is going to break a bunch of our tests
• react-query docs suggest using the AbortSignal API instead of promise.cancel. We might be better switching that too in future.

[atanaskanchev]

@cabljac amazing work, all seems to be in order. Should we as part of this PR bump the react dependencies to 18? I've used it with react 18 for some time now and haven't faced any issues

[cabljac]

OK so last bit of this is to add at least a single test for the error case, but that requires setting up firestore.rules and changing test setup a bit.

[cabljac]

a couple of tests pass on their own, but fail when run in series, and I can't figure out why. I skipped them and merged this. If people get a chance and are able to test it out, that would be greatly appreciated :D