Apollo-client: skip in useQuery hook doesn't work as expected

[JayMGurav]

Intended outcome:

I was trying to make request conditional, using "skip" in "useQuery" hook, here a brief code of that

here the second query GET_COURSE_BY_ID should be "skipped" if the bought is true,
that's well and good when logged in the console, I get courseDetails as undefined if bought is true,
but the thing is it actually makes the network request and sends back the requested data!!

here's what I get in the console

As expected 👍, courseDetails is undefined if bought is true, But as seen in the network tab it still actually makes a network request and return back the data for courseDetails it so happens that it just makes it undefined later;

The first request - for CHECK_COURSE_BOUGHT

The second request -for GET_COURSE_BY_ID

Actual outcome:

The network request should not be made!!, It should be "skipped" else what is the use of using it. if it is later on just reassigning the data to undefined.

How to reproduce the issue:

Just fallow the code above

Versions

System:
OS: Windows 10 10.0.18363
Binaries:
Node: 12.13.1 - C:\Program Files\nodejs\node.EXE
Yarn: 1.17.3 - F:\softwares\yarn\bin\yarn.CMD
npm: 6.12.1 - C:\Program Files\nodejs\npm.CMD
Browsers:
Edge: 44.18362.449.0
npmPackages:
apollo-link-context: ^1.0.20 => 1.0.20
@apollo/client: "^3.0.0-beta.43",

[joonhocho]

I get the same bug. skip is not respected

[haneetsingh]

I also get the same issue with skip not getting evaluated.

[jengel-rackner]

It's failing for me as well

[macrozone]

skip seem to be broken in apollo-client version 3. using ^3.0.0-beta.50

[dobrynin]

+1

[layerssss]

seems still broken in "@apollo/client": "^3.0.0-rc.4"

[buzinas]

I'm using @apollo/client: ^3.0.0-rc-4.

Use cases:

1. Pass true directly, and it correctly skips.
2. Pass false directly, and it correctly triggers an HTTP request.
3. Pass a variable with a true value, and it correctly skips.
4. Pass a variable with a false value, and it correctly triggers an HTTP request.
5. Pass a variable with false, and then change it to true: it should skip, but it doesn't.

Small runnable reproduction:

https://codesandbox.io/s/skip-does-not-work-rx2rj?file=/src/App.js

I have an array of currencies, and I only want to fetch the currencies which indexes are odd.

Expected

Only queries for odd indexes should trigger an HTTP request.

Actual

After setting the variable to false once, it ignores when I change it to true, so all queries trigger an HTTP request afterwards.

Screenshots

Running console.log(data, currency, skip) correctly shows what I'm expecting:

USD is even, so it skips, and data is undefined.
BRL is odd, so it doesn't skip, and data is returned.
EUR is even, so it skips, and data is undefined.
CAD is odd, so it doesn't skip, and data is returned.
AUD is even, so it skips, and data is undefined.
GBP is odd, so it doesn't skip, and data is returned.

Unfortunately, Apollo triggers an HTTP request anyway, independent of skip:

The only one it respects is the first time I pass true (USD), but EUR and AUD it sends an HTTP request anyway, even if I'm telling it to skip, and even it returning me undefined for data.

[buzinas]

As a work-around, I'm using fetchPolicy: skip ? 'cache-only' : 'cache-first', but this is a nasty bug.

[Taraluktus]

Still having this issue (truthy skip values are ignored) with final version 3.0.0

But I was able to bisect it, maybe that helps the AC contributors (@benjamn @hwillson):

9793b7d9159fae3e82f989be3db6d923fd811893 is the first bad commit
commit 9793b7d9159fae3e82f989be3db6d923fd811893
Author: Ben Newman <[email protected]>
Date:   Thu Apr 30 12:04:17 2020 -0400

    Refactor ObservableQuery#reobserve to use Reobserver class.

 src/core/ObservableQuery.ts | 198 +++++++++-----------------------------------
 src/core/QueryManager.ts    |  55 ++++++------
 src/core/Reobserver.ts      | 142 +++++++++++++++++++++++++++++++
 3 files changed, 208 insertions(+), 187 deletions(-)
 create mode 100644 src/core/Reobserver.ts

Bisect range was from tag v3.0.0-beta.45 (good) till tag v3.0.0-beta.46 (bad):

git bisect start
# bad: [884b1e5ac35ccad7c8c7778eeaf1f0d7ad40699a] Bump @apollo/client npm version to 3.0.0-beta.46.
git bisect bad 884b1e5ac35ccad7c8c7778eeaf1f0d7ad40699a
# good: [5bd74df074ccd1ec9273e3f4464db0e195e95af2] Bump @apollo/client npm version to 3.0.0-beta.45.
git bisect good 5bd74df074ccd1ec9273e3f4464db0e195e95af2
# bad: [9793b7d9159fae3e82f989be3db6d923fd811893] Refactor ObservableQuery#reobserve to use Reobserver class.
git bisect bad 9793b7d9159fae3e82f989be3db6d923fd811893
# good: [570cb66f9be66407c72a69dbc1ae415d9acb1a3d] Improve Concast comments.
git bisect good 570cb66f9be66407c72a69dbc1ae415d9acb1a3d
# good: [ae3f6d5a5e055aa17e705b8a25d8972f05b19a85] Go back to using a fresh query ID for fetchMore requests.
git bisect good ae3f6d5a5e055aa17e705b8a25d8972f05b19a85
# good: [8aeeba47199ffa49d32107f5a3a0d1f7c0a15e43] Allow in-flight fetchQueryObservable fetches to be cancelled.
git bisect good 8aeeba47199ffa49d32107f5a3a0d1f7c0a15e43
# good: [23b9d07f4f7127a4d2f5d5af2a457bbc841dfd04] Make Concast#{add,remove}Observer public, and allow quiet removal.
git bisect good 23b9d07f4f7127a4d2f5d5af2a457bbc841dfd04
# good: [6acaa370c819eab6e81d524923911bcf7fa4e2b7] Move polling logic from QueryManager to ObservableQuery.
git bisect good 6acaa370c819eab6e81d524923911bcf7fa4e2b7
# first bad commit: [9793b7d9159fae3e82f989be3db6d923fd811893] Refactor ObservableQuery#reobserve to use Reobserver class.

[sapkra]

If it is the exactly the same problem the fix is already merged and will come in the next release:
https://github.com/apollographql/apollo-client/issues/6122#issuecomment-657906722
https://github.com/apollographql/apollo-client/blob/master/CHANGELOG.md

[Taraluktus]

@sapkra Thanks for mentioning it!
I've only tested until 3.0.0-final, will report back if this fixes it for me.

[hwillson]

This should now be fixed in @apollo/[email protected] (reference: #6589). Let us know if not - thanks!

[Taraluktus]

Unfortunately, this still happens for me with 3.0.1 (fetchPolicy "network-only", skip: true)

[vjsingh]

@hwillson I believe this bug is about stopping the actual request, whereas it appears your fix only changes the behavior of onCompleted.

[buzinas]

Yep, still broken @hwillson.

[Taraluktus]

I am working around this for now with manually setting fetchPolicy to "standby" if the condition for skip is truthy, aditionally to setting skip.
But I'm not sure what "standby" is really meant for, did not found any explanation in the documentation.

Nevermind, "cache-only" did the trick ;-)

[buzinas]

I'm using a similar work-around (using cache-only) to make sure fetch isn't triggered, but this should be fixed.

[buzinas]

This is the commit that broke it, btw:
https://github.com/apollographql/apollo-client/commit/9793b7d9159fae3e82f989be3db6d923fd811893

cc @hwillson, @sapkra ^

The onCompleted hook was a separate issue. Please reopen this.

[MichaelBurgess]

@hwillson - still seeing this as an issue in 3.0.1.

[Taraluktus]

@hwillson Yes, please re-open this.

[brunomarchioro]

Hi @hwillson! This problem appears in my projects too, please re-open this issue.

[sirugh]

This is happening for us as well using 3.0.2.

[Taraluktus]

Any news on this? Could this be re-opened, please?

[Karandashyk]

The is still breaking our app even with workaround.
The behaviour should be to SKIP when fetchPolicy: "network-only", skip: true.
Have all packages updated.
Reopen the issue, please.

[jimisaacs]

Also happening with me with network only same description as @Karandashyk
Upgraded to @apollo/client 3.1.1 without any change.
Edit: Please reopen the issue. It's open :)

[278kunal]

Facing the same issue on 2.6.8

[MichaelBurgess]

Seeing this issue again in 3.2.0. It was good for me in 3.1.5. I've rolled back to 3.1.5 for now.

Should this be re-opened, or shall I raise a new issue?

[Taraluktus]

@MichaelBurgess I don't have the skip issue outlined in this thread at 3.2.0 (had it before 3.1.2), so it might be another issue.

[MichaelBurgess]

@Taraluktus actually I may have found a change in behaviour which has led to this. At the point of seeing the issue, skip was set to true and query was also null, as I don't yet have enough context to know which dynamic query to use, hence skip: true.

In 3.1.5 that would be fine, but seems like in 3.2.0 it barfs if the query is null and skip is true?

Argument of null passed to parser was not a valid GraphQL DocumentNode. You may need to use 'graphql-tag' or another method to convert your operation into a document

I don't see any documented change for that, so I'll raise a new issue in case either the release notes need updating, or it was an unintended side-effect of another change.

[nateq314]

Seeing this here with 3.1.1. What's the latest status?

[EllaSharakanski]

Same here, any news?

[gitpash]

3.2.2 seems fixed it for us

[Hatko]

The issue is still reproducible on 3.2.4

[joshuarobs]

Any updates on the issue?

[jean9696]

We experience the same issue with3.2.4

[Hatko]

@hwillson Should the issue be reopened due to many ppl still experiencing it?

[sirugh]

I would recommend that folks open a new issue with a repro link for codesandbox. Probably the best way to get eyes on something.