v3: Remove refetchOnMount and always fetch when stale

[GeospatialMax]

This PR removes the refetchOnMount option which was used as a condition for refetching/fetching the query. Instead of this the query refetching will depend purely on whether a query is stale, enabled and not suspended.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tannerlinsley/react-query/2ejbw0xcw
✅ Preview: https://react-query-git-fork-geospatialmax-master.tannerlinsley.vercel.app

[tannerlinsley]

As stated in the umbrella issue for v3, I would really love to just get rid of this functionality and have refetchOnMount default to false and remove the single instance condition. This would effectively solve your issue right?

[GeospatialMax]

As stated in the umbrella issue for v3, I would really love to just get rid of this functionality and have refetchOnMount default to false and remove the single instance condition. This would effectively solve your issue right?

Hi @tannerlinsley , sure, that would work too. The end-result for us would be the same. I was somehow under the impression that you wanted to keep the functionality to refetch when only a single instance exists. But removing that extra condition of course would make things easier.

[tannerlinsley]

The more I think about it, it should be simpler. Something like:

  if (
    query.config.enabled && // Must be enabled
    !query.wasSuspended && // Cannot be in a suspended state
    isStale // Must be stale
  ) {
    await query.fetch()
  }

[GeospatialMax]

The more I think about it, it should be simpler. Something like:

  if (
    query.config.enabled && // Must be enabled
    !query.wasSuspended && // Cannot be in a suspended state
    isStale // Must be stale
  ) {
    await query.fetch()
  }

Mmh, I have tested it like you proposed and that wouldn't resolve the issue. The query would refetch like it was doing before. If we were to replace the construct with neverRefetchOnMount and the other conditions, i.e. this bit below

      // Don't refetch on mount when 'neverRefetchOnMount' is set, otherwise only refetch if either only one instance of the query exists or 'refetchOnMount' is set
      const shouldRefetchOnMount = query.config.neverRefetchOnMount ? false : (query.config.refetchOnMount || query.instances.length === 1);

      if(
        (!query.state.isSuccess || shouldRefetchOnMount) && // Make sure first load happens, thereafter only if refetch on mount is requested
        query.config.enabled && // Don't auto refetch if disabled
        !query.wasSuspended && // Don't double refetch for suspense
        query.state.isStale // Only refetch if stale
        
      ) {
        await query.fetch();
      }

with your suggestion or with

if(
        query.config.refetchOnMount &&
        query.config.enabled && // Don't auto refetch if disabled
        !query.wasSuspended && // Don't double refetch for suspense
        query.state.isStale // Only refetch if stale
        
      ) {
        await query.fetch();
      }

then it would either not load the data at all in the case above, or in case of your suggestion it would ignore that there's already data and refetch anyway every time a new instance comes along (mind this is for the case where you had a query instance before that stored the returned data in the query cache, then was unmounted and remounted before the queryCache was garbage-collected).

[tannerlinsley]

So I guess a better question would be: If you don't want a query to refetch, then why not tweak the staleTime?

[GeospatialMax]

So I guess a better question would be: If you don't want a query to refetch, then why not tweak the staleTime?

Good point, I hadn't considered that option. So the default stale time is 0 I suppose? I have tested it with setting it to 5 minutes and it seems to resolve the issue, i.e. the query is not reloaded if it's already in the query cache.

In that case we could just move to

  if (
    query.config.enabled && // Must be enabled
    !query.wasSuspended && // Cannot be in a suspended state
    isStale // Must be stale
  ) {
    await query.fetch()
  }

and everything should work fine (it would without that change too as the isStale will be false with a high staleTime in most cases, but would just simplify things I suppose)

[tannerlinsley]

This would be my preference as outlined in the v3 umbrella issue :)

[GeospatialMax]

This would be my preference as outlined in the v3 umbrella issue :)

Sounds good. I would be happy to update this PR to reflect that. Or do you want to do make those changes yourself at some point later?

[tannerlinsley]

Switch the PR for now. We may keep it open until we're ready to start making v3 changes. It could be a while.

[tannerlinsley]

Given the amount that the library has changed from this PR, we'll be opening a new PR for v3 to address this. Thanks for your help!

[GeospatialMax]

Given the amount that the library has changed from this PR, we'll be opening a new PR for v3 to address this. Thanks for your help!

Sure thing. Thanks for keeping it on the agenda 👍