Refetch no longer works in 3.5 when skip is true

[patrickconroy]

Hello. I've been using refetch from useQuery for over a year. In my code, I use it in combination with skip: true to fetch queries lazily. Starting from 3.5 (worked in 3.4), I can no longer do that. My browser doesn't run the query as expected with the following code:

import { Query } from './something.graphql';

const Test = () => {
    const variables = { ... };
    const { refetch } = useQuery(Query, {
        skip: true,
        variables,
    });
    const click = async () => {
        const response = await refetch();
        console.log('response', response);
    };

    return <button onClick={() => click()}>Refetch</button>;
};

Intended outcome:
A network request would be made for the query, and response would include the results of that.

Actual outcome:
No query is made andresponse is undefined.

How to reproduce the issue:
Code above.
I'm assuming this might have to do with skip: true, but again this worked < 3.5.

Versions
System:
OS: macOS 11.6
Binaries:
Node: 17.0.1 - /usr/local/bin/node
npm: 8.1.0 - /usr/local/bin/npm
Browsers:
Chrome: 95.0.4638.69
Safari: 15.1
npmPackages:
@apollo/client: ^3.3.21 => 3.5.4

[brainkim]

Sorry to hear the behavior changed in an unexpected way. So the ask is, allow refetch() to work even if skip is true? Seems reasonable I guess.

[patrickconroy]

Thanks for getting back to me so quickly. I'm not averse to updating my code to what the correct way to do it is (I assume useQueryLazy? I don't remember at this point if the skip/refetch method was something I found in someone's blog or if that used to be in the docs.

[sebranly]

You may have found it in this comment? apollographql/react-apollo#3499 (comment)

[sebastiansanio]

Is this expected or it's going to be reverted?

[brainkim]

@sebastiansanio We’ll re-enable this “use-case!”

[okbytes]

My experience using refetch with useQuery is that it executes but the loading boolean does not update unless notifyOnNetworkStatusChange: true is set on the query. I have a dynamic prop that sets skip. I'm on 3.5.5.

[JensMadsen]

I have the same bug as reported by @patrickconroy. Setting notifyOnNetworkStatusChange: true does not solve my issues @01brett. I have reverted to 3.4.

[brainkim]

I got a very funny story for you guys. There’s another issue which says that calling refetch() when skip is true shouldn’t cause a network request (#8270) by @knedev42, essentially, the opposite expectation of this issue. I discovered this because I found a unit test I had written which started failing when I tried to “fix” this issue.

Do you guys wish to debate which behavior is “expected”? Because I am more or less disinterested on this issue. I’m leaning towards reverting to the old behavior because that would make a bunch of useLazyQuery() 3.5 bugs easier to fix.

[okbytes]

@brainkim I'm leaning toward refetch() executing regardless of skip. My use for skip is to avoid automatic useQuery execution on mount in certain conditions, such as, loading an object's configuration (skip: false because I want to load that data) vs creating a new object configuration (skip: true because I don't have data to load). I'm using the same component for creation/editing of a particular object in our application. (A web form)

[JensMadsen]

In any case dont we consider this a breaking change? If so this is not according to semver IMHO.

[cjreimer]

My opinion is that refetch should always execute regardless of skip. This change broke our application.

[enheit]

I faced with the same issue after updating to the @apollo/client@^3.5.6. (At @apollo/client@^3.4.16 everything worked as expected)

I use const { refetch } = useQuery(...) + skip: true to make async validation of the form field. Now it returns undefined and throws an error on the screen

Unhandled Rejection (TypeError): Cannot destructure property 'data' of '(intermediate value)' as it is undefined.

[zant]

My opinion is that refetch should always execute regardless of skip. This change broke our application.

+1

I was working with refetch + skip: true for imperative-style calls. I also have the opinion that skip should just skip the first automatic call made, but not subsequent on-demand ones.

[FennyFatal]

+1 refetch should always trigger a network call.

[patrickconroy]

I’ve thought about this for a while over the holidays and I think that I shouldn’t have to conditionally call useQuery or useLazyQuery, because that’s cumbersome. A useQuery should become lazy if it’s skip:true. If I need to have the query run (or not run) based on some prop, I don’t want to, and don’t think others should, write an if/then for that. The 3.4 and below way for handling async’ing a query was pretty great, and even if it wasn’t something the main docs talked about, it made it possible to do a bunch of cool stuff.

Since moving from 3.4 to 3.5 this was a breaking change, the default should be reverted and a new skipAlways could be added for folks who need the new behavior.
If that’s not possible, then a new declarative skip behavior should be added.. something like skipWithRefetch.. but that might be confusing too.

Although I’ll ask: if a user explicitly makes skip:true, but they’re also calling refetch, why was there the 3.5 change at all to then ignore that refetch? I get that the prefix “re” then is incorrect but I feel that created more problems than it solved. If your code is saying “run this query”, then it should run.

[ctataryn]

Just wanted to pop in and say "thanks" for this fix! I have been struggling to upgrade an v2 app to v3 over the last week. refetch was always returning undefined using ^3.5.7. Out of sheer luck I blew away my yarn.lock file today and when I restarted the app, everything worked as normal! I tracked it down to the app now using v3.5.8 and when I sifted through the commits between v3.5.7 and v3.5.8 I landed here and this issue described the problem I was facing all along.

What a difference a day can make, eh?