react-query-firebase icon indicating copy to clipboard operation
react-query-firebase copied to clipboard

useFirestoreDocument/useFirestoreQuery with { subscribe: true } are not subscribing to realtime updates on remount

Open ifours opened this issue 2 years ago • 71 comments

I've discovered a weird behaviour of useFirestoreDocument/useFirestoreQuery hooks: { subscribe: true } option will have no effect, if query is remounted (in default cacheTime 5 mins window), after became inactive. Probably, It will be good, if query will resubscribe to realtime changes, if there is a snapshot in a cache and there are no active subscribers.

As a workaround we are setting cacheTime to 0, so query will trigger queryFn again and put active subscription on firestore query.

ifours avatar Nov 16 '21 17:11 ifours

Interesting I'll check this scenario out.

When subscribe is true, the staleTime is set to infinite - because it'll always be kept up to date from Firebase. I would have thought that if the data is cached, it'd show the cached data and re-create the subscription from that - or at least that was my understanding of React Query.

Ehesp avatar Nov 16 '21 17:11 Ehesp

Yeap, queryFn will never be called if data exist in cache for staleTime infinite. But when useFirestoreDocument or useFirestoreQuery for a given key is unmounted, useEffect unsubscribes from realtime updates, while the only way to subscribe again is to call queryFn somehow, which will not happen, while cache exists.

ifours avatar Nov 16 '21 17:11 ifours

Don't know if it helps, but I had some issues with caching with react-query in the past. Try these settings while setting up your queryClient

queries: {
  refetchOnWindowFocus: false,
  staleTime: 10 * 60 * 1000, // 10 min - had to set this explicitly!
  cacheTime: 5 * 60 * 1000, // 5 min (default)
}

so in our case it would look like something like this

const queryClient = new QueryClient({
  defaultOptions: {
    queries: {
      retry: 3,
      retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000),
      staleTime: 10 * 60 * 1000, // 10 min
      cacheTime: 5 * 60 * 1000, // 5 min (default)
      refetchOnWindowFocus: false,
    },
  },
});

julianklumpers avatar Nov 16 '21 18:11 julianklumpers

@ifours Oh dang you're right, that situation didn't occur to me.

Maybe the idea of Infinity for realtime data isn't the best idea. In my head it was a way to only ensure there was a single listener for a Query Key. Maybe an alternative option would be to keep the defaults, however store a query reference in memory, and for each new query, check if there's an active subscription.

That or just create a new one each time, I'm not sure what's best here.

Ehesp avatar Nov 16 '21 19:11 Ehesp

@julianklumpers thanks for the advice. But without setting cacheTime: 0 subscribe option is still not working properly on remount.

@Ehesp there is a meta option in useQuery. It could store additional information on the query cache entry like onSnapshot callback (for resubscribing on remount) and unsubscribe function. Do you think it will be helpful?

ifours avatar Nov 16 '21 20:11 ifours

I'll need to dig into this - my concern is that even storing data within meta won't be useful since the useQuery hook simply never fires.

If set to Infinity, the data will never be considered stale

I guess this means, the data in cache is also never considered stale, hence the query function never firing again.

Right now, if the hook which created the initial query (and contains the unsubscribe reference) unmounts, the subscription will also be unsubscribed from. I'm struggling to see a way to re-trigger the subscription again.

The reason I decided to make the stale time Infinity is because when Firestore is providing realtime updates, the data will never be stale. However once all usages of a hook are not in use, it does become stale. The problem is, I can't leave the subscription open since it's wasted bandwidth / reads.

Ehesp avatar Nov 17 '21 16:11 Ehesp

https://github.com/tannerlinsley/react-query/discussions/2962

Based on the comment here, setting refetchOnMount to true if subscribed might be the fix!

Ehesp avatar Nov 17 '21 18:11 Ehesp

Ok yep I have tested in a sandbox and pretty sure that will work - you can try this yourself without a release:

useFirestoreDocument([...], doc, {
  subscribe: true,
}, {
  refetchOnMount: "always",
});

I'll get a release up tomorrow which basically sets refetchOnMount: "always" if subscribe: true. If anyone could test this out in the meantime please do!

Ehesp avatar Nov 17 '21 19:11 Ehesp

While refetchOnMount: "always" will solve the problem with resubscribing, this approach could create multiple subscriptions to the same document if useFirestoreDocument hooks are not rendered in the same time (for example some components with this hook could be rendered later if some conditions applied)

Probably, cacheTime: 0 will be more optimal solution in terms of firebase billing.

ifours avatar Nov 17 '21 19:11 ifours

How do you measure the amount of subscriptions on a document? Maybe we could do something with a hash of the query key and see if there is already an active subscription?

julianklumpers avatar Nov 17 '21 20:11 julianklumpers

I'll have a play around tomorrow, this is a interesting problem at least! Thanks for the insights.

My reservation with no cache is you lose the benefit of instant data showing while navigating around the application. Instead it would be loading each time.

Ehesp avatar Nov 17 '21 22:11 Ehesp

Ok I think I need to rework how subscriptions are handled. Non-subscription (gets) are fine as they are, however for subscriptions I think I need to do something such as:

  1. Create the hash of the user provided key using hashQueryKeyByOptions
  2. Create a useEffect, which accepts that hashed key.
  3. Store the hash in a global mutable object, counting the renders for a given hash.
  4. If the count is 0, start a subscription (and store it against the hash). When the data returns, update the query cache using the query client.
  5. If the count is > 0, a subscription already exists so do nothing.
  6. In the effect unmount, decrease the counter for the key, if it now is 0, unsubscribe the subscription.
  7. Each time a RQF hook is mounted, within the useQuery fn, somehow detect whether the subscription is active (if it is, return the data, otherwise wait for the subscription to trigger and return the data)

Ehesp avatar Nov 19 '21 12:11 Ehesp

@Ehesp hello, any news about improvements?

vomchik avatar Dec 02 '21 19:12 vomchik

It's on my backlog for next week - sorry been a crazy couple of weeks.

Ehesp avatar Dec 08 '21 12:12 Ehesp

any update on this issue?

princejoogie avatar Dec 18 '21 12:12 princejoogie

In progress!

Ehesp avatar Dec 18 '21 17:12 Ehesp

Ok sorry for the delay here. I've attempted a solution a few times but kept getting frustrated. To keep things simple, I decided to try the solution on the useAuthUser hook, since it's fairly basic and is always in a subscription state.

import { useEffect, useRef } from "react";
import {
  hashQueryKey,
  QueryKey,
  useQuery,
  useQueryClient,
  UseQueryOptions,
  UseQueryResult,
} from "react-query";
import { Auth, User, Unsubscribe, AuthError } from "firebase/auth";
import { Completer } from "../../utils/src";

const counts: { [key: string]: number } = {};
const subscriptions: { [key: string]: Unsubscribe } = {};

export function useAuthUser<R = User | null>(
  key: QueryKey,
  auth: Auth,
  useQueryOptions?: Omit<UseQueryOptions<User | null, AuthError, R>, "queryFn">
): UseQueryResult<R, AuthError> {
  const client = useQueryClient();
  const completer = useRef<Completer<User | null>>(new Completer());

  const hashFn = useQueryOptions?.queryKeyHashFn || hashQueryKey;
  const hash = hashFn(key);

  useEffect(() => {
    counts[hash] ??= 0;
    counts[hash]++;

    // If there is only one instance of this query key, subscribe
    if (counts[hash] === 1) {
      subscriptions[hash] = auth.onAuthStateChanged((user) => {
        // Set the data each time state changes.
        client.setQueryData<User | null>(key, user);

        // Resolve the completer with the current data.
        if (!completer.current!.completed) {
          completer.current!.complete(user);
        }
      });
    } else {
      // Since there is already an active subscription, resolve the completer
      // with the cached data.
      completer.current!.complete(client.getQueryData(key) as User | null);
    }

    return () => {
      counts[hash]--;

      if (counts[hash] === 0) {
        subscriptions[hash]();
        delete subscriptions[hash];
      }
    };
  }, [hash, completer]);

  return useQuery<User | null, AuthError, R>({
    ...useQueryOptions,
    queryKey: useQueryOptions?.queryKey ?? key,
    queryFn: () => completer.current!.promise,
  });
}

I've tested this out and it seems to work - however can anyone see any obvious problems? Basically:

  1. Each time the hook mounts, I count based off the query key.
  2. If that key doesn't exist yet in the global state, increment a counter, start a subscription and resolve the deferred promise.
  3. If the key exists and the count is greater than 1, resolve the deferred promise with the cached value.
  4. Return the promise from the query function. Any updates will then update the RQ query cache.
  5. If the hook unmounts and there is no longer any active for a given key, unsubscribe.

I need to figure out how to test this as well :D

Ehesp avatar Jan 05 '22 11:01 Ehesp

Hi @Ehesp, thanks for your effort, much appreciated. Would it be possible to cut off a beta release to give it a test?

atanaskanchev avatar Jan 09 '22 22:01 atanaskanchev

Hey, do you have any updates about this issue?

tutizaraz avatar Jan 17 '22 15:01 tutizaraz

Possibly related to this issue. I am still experience it and is a show stopper for me.

atanaskanchev avatar Jan 17 '22 21:01 atanaskanchev

https://github.com/invertase/react-query-firebase/pull/35

WIP PR here

Ehesp avatar Jan 26 '22 12:01 Ehesp

@Ehesp Looks like I have found another way how we can fix it.

useEffect(() => {
    if (options.subscribe && unsubscribe.current) {
      client.refetchQueries(key);
    }
    // We need to run it only once, so let's ignore deps
  }, []);

So it should call a queryFn again. For me, it works fine. Do you see any problems here?

vomchik avatar Jan 26 '22 21:01 vomchik

Hmm I think the issue is also that the ref holding the unsubscribe function is only stored on the first hook, not subsequent ones, so additional hooks would never trigger the refetch?

I also think there is an additional issue with the existing setup whereby if you have 2 hooks using the same query key, imagine the following scenario:

  • Hook 1 mounts & creates + holds the subscription in a ref & keeps the cache fresh
  • Hook 2 mounts, starts to use the cached data
  • Hook 1 unmounts and with it also since it holds the ref, it triggers the un-subscription
  • Hook 2 is left in limbo listening to a cache which will never update

I think the PR also handles this scenario.

Ehesp avatar Jan 27 '22 10:01 Ehesp

Hello @Ehesp. There's a similar issue with useDatabaseSnapshot, it stops reflecting rtdb changes after a while. Should I create a ticket for that?

greendesertsnow avatar Jan 28 '22 17:01 greendesertsnow

@greendesertsnow It's the same issue, the PR should address it so no need to open another issue.

Ehesp avatar Jan 28 '22 17:01 Ehesp

Hey! Just landed here because I'm having the same issue. Please let me know if you need help with testing or fixing this (would love to use this lib and this is a no go on our case)

refetchOnMount: 'always', doesn't seem to be working (as a workaround) every time

estebanrao avatar Mar 15 '22 14:03 estebanrao

Actually refetchOnMount: 'always', is always working, my bad. Still, it creates a lot of unnecessary reads.

estebanrao avatar Mar 15 '22 20:03 estebanrao

Is this bug fixed or it still persists?

T4533N avatar Apr 20 '22 06:04 T4533N

Is this bug fixed or it still persists?

It still persists

bojanvidanovic avatar May 06 '22 11:05 bojanvidanovic

Hey, going to take a look at this :)

cabljac avatar May 19 '22 12:05 cabljac