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

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

Open
ifours opened this issue Nov 16, 2021 · 71 comments · Fixed by #35
Assignees
Labels
bug Something isn't working

Comments

@ifours
Copy link

ifours commented Nov 16, 2021

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.

@Ehesp
Copy link
Member

Ehesp commented Nov 16, 2021

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 Ehesp added the bug Something isn't working label Nov 16, 2021
@ifours
Copy link
Author

ifours commented Nov 16, 2021

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.

@julianklumpers
Copy link

julianklumpers commented Nov 16, 2021

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,
    },
  },
});

@Ehesp
Copy link
Member

Ehesp commented Nov 16, 2021

@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.

@ifours
Copy link
Author

ifours commented Nov 16, 2021

@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?

@Ehesp
Copy link
Member

Ehesp commented Nov 17, 2021

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
Copy link
Member

Ehesp commented Nov 17, 2021

TanStack/query#2962

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

@Ehesp
Copy link
Member

Ehesp commented Nov 17, 2021

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!

@ifours
Copy link
Author

ifours commented Nov 17, 2021

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.

@julianklumpers
Copy link

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?

@Ehesp
Copy link
Member

Ehesp commented Nov 17, 2021

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
Copy link
Member

Ehesp commented Nov 19, 2021

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)

@vomchik
Copy link
Contributor

vomchik commented Dec 2, 2021

@Ehesp hello, any news about improvements?

@Ehesp
Copy link
Member

Ehesp commented Dec 8, 2021

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

@princejoogie
Copy link

any update on this issue?

@Ehesp
Copy link
Member

Ehesp commented Dec 18, 2021

In progress!

@Ehesp
Copy link
Member

Ehesp commented Jan 5, 2022

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

@atanaskanchev
Copy link

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

@dbarabashh
Copy link

Hey, do you have any updates about this issue?

@atanaskanchev
Copy link

atanaskanchev commented Jan 17, 2022

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

@Ehesp
Copy link
Member

Ehesp commented Jan 26, 2022

#35

WIP PR here

@vomchik
Copy link
Contributor

vomchik commented Jan 26, 2022

@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?

@Ehesp
Copy link
Member

Ehesp commented Jan 27, 2022

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.

@greendesertsnow
Copy link

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

@Ehesp
Copy link
Member

Ehesp commented Jan 28, 2022

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

@estebanrao
Copy link

estebanrao commented Mar 15, 2022

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
Copy link

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

@cabljac
Copy link
Contributor

cabljac commented Jun 29, 2022

@0xR

Previously were you able to pass undefined in as a query/named query? It shouldn't run the query in that case i agree

@0xR
Copy link

0xR commented Jun 29, 2022

@0xR

Previously were you able to pass undefined in as a query/named query? It shouldn't run the query in that case i agree

Indeed, that worked previously, but doesn't work anymore.

@cabljac
Copy link
Contributor

cabljac commented Jun 29, 2022

@0xR

and apologies for more questions, quite new to working on this library. What behaviour would you expect with an undefined ref but enabled:

  const query = useFirestoreQueryData('testkey', undefined, { subscribe: false }, { enabled: true });

@0xR
Copy link

0xR commented Jun 29, 2022

I expect an error with a descriptive error message.

new Error('useFirestoreQueryData with key ["testkey"] expected to recieve a document reference, but got "undefined". Did you forget to set the options "enabled" to false?'

@cabljac
Copy link
Contributor

cabljac commented Jun 30, 2022

OK 1.0.0-dev.6 for firestore, think i've addressed these issues, let me know if you still experience them :)

I'll probably have to do similar error handling for the other packages. Thanks again for testing this out, really appreciate it.

@0xR
Copy link

0xR commented Jul 4, 2022

I'm running dev.6 now and the issue above are solved!

However I'm running into an issue where it seems like a firestore subscription is stopped while it should still be subscribed.

I'm trying to edit some data with 2 different windows. The edit view and the regular view are subscribed to the same data. The edits show up on the other window, but not on the window you are editing on. It seems like the once you click save the subscription to the session data is stopped.

Refreshing the page solved the issue.

In another Chrome browser it does seem to work. It could be a race condition. I suspect because the edit view is unmounted the subscription is stopped, but then the detail view is immediately shown and the subscription should be running again.

The video below has some issues. But it might be useful.
Kooha-07-04-2022-22-59-05.webm

@estebanrao
Copy link

estebanrao commented Jul 7, 2022

I'm trying this fixes but now data will be null if I have subscribe: true

  const { isLoading, data: dataContacts } = useFirestoreQueryData(
    'contacts',
    ref,
    {
      subscribe: true,
      idField: 'id',
    },
    {
      // Momentary fix for https://github.com/invertase/react-query-firebase/issues/25
      // refetchOnMount: 'always',
    }
  );

If I comment it out it will work but of course... I won't get real time updates

@estebanrao
Copy link

estebanrao commented Jul 7, 2022

If I change something from that collection in firestore it starts working, but not on first load.
It also seems to load again if I navigate away and back from the page I'm fetching the collection

@estebanrao
Copy link

Doing some more debugging I can see that once I load, subscribeFn in useFirestoreQueryData will run and the onSnapshot has all the docs in my collection. But once it finishes processing, the initial data array is null.

Also something else I notice, is that if you have an inactive (collection) query and changes are made to a doc, the query will update twice: first when the changes are made, and a second time when the query becomes active again. I don't know if this is a functionality of react query itself but I'd expect the query to update just once. This is still better to have to reload it every time the component remounts though :)

@0xR
Copy link

0xR commented Jul 10, 2022

I added some debug info react-query-firebase. Turns out react-query-firebase thinks no component is listening anymore and unsubscribes to the data. This seems to be due to an error in the reference counting. React-query itself does have a correct reference count. Maybe it's possible to unsubscribe when react-query determines data is not needed anymore opposed to keeping an additional reference count in react-query-firebase?

In this video you see that react-query unsubscribes while the data is still shown. The react-query devtools shows the data always stays referenced with a reference count of 1. There is also a mount / unmount debug log that shows that different components will be listening to the same data. There are route changes in the example.

react-query-firestore-2022-07-10_19.29.52.mp4

@0xR
Copy link

0xR commented Jul 10, 2022

I can't get it to work right now, but we could replace the subscription counting logic with something like this:

const queryHash = hashFn(queryKey);

queryClient.getQueryCache().subscribe((event) => {
  if (event?.type === "observerAdded" || event?.type === "observerRemoved") {
    if (event.query.queryHash === queryHash) {
      const observersCount = event.query.getObserversCount();
      if (observersCount === 0) {
        console.log("unsubscribe", queryHash);
      } else {
        console.log("subscribe", queryHash);
      }
    }
  }
});

I'm not sure whether this should run in a useEffect or just within the render loop since some state is managed outside React.

@cabljac
Copy link
Contributor

cabljac commented Jul 12, 2022

Sorry i've been less responsive, was taken up with other work for a bit. Will catch up today.

@tombouquet
Copy link

Any updates?

@cabljac
Copy link
Contributor

cabljac commented Aug 4, 2022

I've published new dev releases for the packages just now, i still need to do some manual testing i think.

@cabljac
Copy link
Contributor

cabljac commented Aug 17, 2022

If anyone has some spare time to sanity check the dev releases, would be very grateful! I will doing this now, and if there are no problems, i'll publish a non-dev release

@reimertz
Copy link

reimertz commented Aug 26, 2022

Thanks so much for the effort creating this!

I've had a really good experience so far, everything working without a problem, til now.

it seems subscribe doesn't work when using useFirestoreDocumentData

Here is an example

import { useFirestoreDocumentData } from '@react-query-firebase/firestore'
import { doc } from 'firebase/firestore'

import { getFirebase } from '@firebase/index'

export const useUser = (userId, { subscribe = true } = {}) => {
  const db = () => getFirebase().firestore()
  const ref = doc(db(), 'users', userId)

  const { data, isLoading, error } = useFirestoreDocumentData(['user', { userId }], ref, {
    idField: 'id',
    subscribe,
  })

  return { user: data,  isLoading, error }
}

versions

 "@react-query-firebase/auth": "^1.0.0-dev.2",
 "@react-query-firebase/firestore": "^1.0.0-dev.7",
 "react-query": "^3.39.2",

@ItsGPhillips
Copy link

Any more updates? { subscribe: true } still isn't working for me

@atanaskanchev
Copy link

Any more updates? { subscribe: true } still isn't working for me

Unfortunately it seems this library is no longer maintained. I've started looking for alternatives and it seems like reactfire is the only suitable alternative for my app.

@Ehesp
Copy link
Member

Ehesp commented Sep 22, 2022

As mentioned, we're really busy with some projects at the moment. Unfortunately since this is OSS we've not got resource right now. Once that becomes available we'll continue to work on it.

@jasuno
Copy link

jasuno commented Oct 11, 2022

Im getting this same problem in next.js but only when remounting or navigating directly to that page. Any solution for that?

Screen.Recording.2022-10-11.at.3.34.35.PM.mov

@mattcoker
Copy link

As this is the only real React/Firebase hook library out there that has full query + mutation capabilities baked in, I think the realtime subscriptions should be considered critically important.

This package is important enough that I can spend a few days digging into this issue to try to solve the subscription bug.

@Ehesp @cabljac this thread hasn't been totally updated with what things were fixed/patched in the various releases related to this issue over the last year. Would one of you be able to provide a little technical context on the current state of this issue so I can make the most of my time debugging this?

@mattcoker
Copy link

Also, if anyone at all could provide some details on how I could get set up with this repo locally to start testing things out, I would really appreciate it. I've never contributed to a package in a monorepo before, and couldn't find documentation in this project about how to contribute.

@dijour
Copy link

dijour commented Dec 21, 2022

love this little package — any traction on this issue? this package is very useful, but the lack of subscription features is killing it for me. Let me know how I can help!

@gtrak
Copy link

gtrak commented Dec 21, 2022

I switched to using reactfire for subs and sticking with react-query-firebase for the extra write hooks, and it's working great, but it would be nice to use one lib.

Ironically, I immediately hit an issue with their query caching that explicit query keys could solve: FirebaseExtended/reactfire#560 (comment)

@dijour
Copy link

dijour commented Dec 21, 2022

I switched to using reactfire for subs and sticking with react-query-firebase for the extra write hooks, and it's working great, but it would be nice to use one lib.

Ironically, I immediately hit an issue with their query caching that explicit query keys could solve: FirebaseExtended/reactfire#560 (comment)

Lol this is somewhat depressing. Just trying to do something very simple and running into huge blockers.

@avand
Copy link

avand commented Mar 23, 2023

+1 I'd love to see this fixed. But it seems to be working in production FWIW. Just local dev is screwy: I have to refresh the page to get it to work. But it seems to be listening to changes just fine. I suppose if we get into a world where the component remounts in production, we will have problems.

@OrkhanAlikhanov
Copy link

I wonder if #73 is related

@ramizqazi
Copy link

Hey guys i just tested the same query on react-native and its working but its not working on the web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.