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

Query Functionality with onCompleted is broken in v3.0.0 release (CodeSandbox repro) #6636

Closed
Tracked by #8596
3nvi opened this issue Jul 17, 2020 · 52 comments
Closed
Tracked by #8596
Labels
🔍 investigate Investigate further

Comments

@3nvi
Copy link

3nvi commented Jul 17, 2020

Some core lazy querying functionality through useLazyQuery seems a bit off.

Specifically, when a lazy query (through a useLazyQuery hook) returns an error, then Apollo seems to start endlessly querying the server. The same thing happens if you give it a fetchPolicy of network-only (or anything that doesn't search the cache first).

Related: apollographql/react-apollo#4044

At the same time, if the lazy query function (returned by useLazyQuery) gets called more than 1 times, then no callbacks are fired. The onCompleted doesn't fire again. The onCompleted fires on the 1st successful run of the "query" function and then consecutive invocations of the lazy query function, don't trigger onCompleted callback (they should, since you can't have conditional business logic based on whether the data is cached or not!)

Related: apollographql/react-apollo#4000

Funnily enough, using client.query() works fine. All this used to work perfectly during the beta (I think I've verified that something occurred after @apollo/[email protected]), but now it doesn't.

UPDATE: I've added a codesandbox that reproduces it

Intended outcome:

  • Lazy query errors shouldn't trigger endless API querying
  • A network-only fetch policy shouldn't trigger endless API querying on a lazy query
  • onCompleted should fire every time a lazy query gets successfully executed

Actual outcome:

  • Lazy query errors & network-only fetch policy start an endless loop of requests
  • onCompeted gets fired at most once regardless of the number of invocations

How to reproduce the issue:

  • Take a lazy query and change the fetchPolicy to network-only or force an API error on a lazy query
  • Click a button that triggers a lazy query multiple time, while observing the number of invocations of the onCompleted callback.

Versions
System:
OS: macOS 10.15.2
Binaries:
Node: 12.18.0 - /usr/local/bin/node
Yarn: 1.19.0 - /usr/local/bin/yarn
npm: 6.14.4 - /usr/local/bin/npm
Browsers:
Chrome: 83.0.4103.116
Firefox: 72.0.2
Safari: 13.0.4
npmPackages:
@apollo/client: ^3.0.2 => 3.0.2
apollo-link-error: ^1.1.12 => 1.1.13

@3nvi
Copy link
Author

3nvi commented Jul 17, 2020

@benjamn Take a look here and observe the console

https://codesandbox.io/s/apollo-reference-equality-1exes?file=/src/App.js

While fetchPolicy is network-only React re-renders endlessly. If you make it cache-first things are ok again. Same thing would happen if the query errored.

Also observe how the ON COMPLETED text gets only logged once regardless of how many times the query finishes successfully

@3nvi
Copy link
Author

3nvi commented Jul 18, 2020

I noticed that even if I change the variables after one query execution, the query automatically executes and fires its onCompleted (although it's lazy, so it shouldn't).

Related to apollographql/react-apollo#3729

In general, this seems like a bug that may be unrelated to lazy querying, but I feel it's something on the "core" functionality of querying since it manifests in many different ways

@3nvi 3nvi changed the title Lazy Load Functionality Broken in v3.0.0 release Lazy Load Functionality Broken in v3.0.0 release (CodeSandbox repro) Jul 18, 2020
@3nvi 3nvi changed the title Lazy Load Functionality Broken in v3.0.0 release (CodeSandbox repro) Lazy Query Functionality Broken in v3.0.0 release (CodeSandbox repro) Jul 18, 2020
@3nvi 3nvi changed the title Lazy Query Functionality Broken in v3.0.0 release (CodeSandbox repro) (Lazy) Query Functionality Broken in v3.0.0 release (CodeSandbox repro) Jul 18, 2020
@kainanaina
Copy link

I'm using 3.0.0-beta.43 since February and everything worked there just fine. Tried an upgrade to stable v3 and encountered this issue.

@orrybaram
Copy link

I've noticed that this is also happening with a regular useQuery hook when an onCompleted function is specified (even if its a noop) and the fetchPolicy is set to anything except cache-first or cache-only

@3nvi
Copy link
Author

3nvi commented Jul 22, 2020

I've noticed that this is also happening with a regular useQuery hook when an onCompleted function is specified (even if its a noop) and the fetchPolicy is set to anything except cache-first or cache-only

Yeah I also mentioned that I feel that this is not necessarily tied only to lazy queries. Will update description

@3nvi 3nvi changed the title (Lazy) Query Functionality Broken in v3.0.0 release (CodeSandbox repro) Query Functionality with onCompletted is broken in v3.0.0 release (CodeSandbox repro) Jul 22, 2020
@3nvi 3nvi changed the title Query Functionality with onCompletted is broken in v3.0.0 release (CodeSandbox repro) Query Functionality with onCompleted is broken in v3.0.0 release (CodeSandbox repro) Jul 22, 2020
@mfgredo
Copy link

mfgredo commented Jul 22, 2020

Same problem. Endless request loop when using onCompleted in one of my applications after upgrading. Might go back to apollo-boost for now.

@hwillson
Copy link
Member

This issue was fixed in #6588, and will be coming in @apollo/[email protected] very soon.

@benjamn
Copy link
Member

benjamn commented Jul 23, 2020

Heads up: we're not quite ready to publish @apollo/[email protected] yet, but I've published an @apollo/[email protected] release that includes #6588, if you want to try that in the meantime. 🙏

Update: thanks to @3nvi's reproduction, I can confirm the fix: https://codesandbox.io/s/apollo-reference-equality-t0ttx

@3nvi
Copy link
Author

3nvi commented Jul 23, 2020

Heads up: we're not quite ready to publish @apollo/[email protected] yet, but I've published an @apollo/[email protected] release that includes #6588, if you want to try that in the meantime. 🙏

Update: thanks to @3nvi's reproduction, I can confirm the fix: https://codesandbox.io/s/apollo-reference-equality-t0ttx

Perfect. Thanks a lot @benjamn !

@emjaksa
Copy link

emjaksa commented Jul 24, 2020

@benjamn I just tried the latest version @apollo/[email protected] and @apollo/[email protected]. When I try execute the search below the query gets stuck in an infinite loop, making multiple network requests and never calling onComplete. If I comment out either the fetchPolicy or onCompleted it stops but the combination of both fetch fetchPolicy: 'no-cache' and onCompleted: data => {} cause the query to go into an infinite loop.

I tried replicating it in your sandbox but I can only replicate it with @apollo/[email protected].

const [search] = useLazyQuery(SearchQuery, {
    fetchPolicy: 'no-cache',
    onCompleted: data => {},
  })

const handleClick = () => { 
  search({
    variables: {
       query: 'search',
    }
  }) 
}

@Tautorn
Copy link

Tautorn commented Jul 24, 2020

@apollo/[email protected]

Thank you so much @benjamn ! It's working for me, great!

@cyberwombat
Copy link

cyberwombat commented Jul 25, 2020

@benjamn I just tried @apollo/[email protected] and it's still a no go. This issue might be slightly different than the 2 year old one (auto archived without fix and reopened by me apollographql/react-apollo#3968 then repo archived). But the gist for me is that onCompleted doesn't fire again rendering the concept of a cache useless. I am a bit confused how anyone is able to use a cache as we never have been able to. @Tautorn you say its working for you - is the cache enabled? how are you adding business logic when refetching from cache?

@emjaksa
Copy link

emjaksa commented Jul 27, 2020

@apollo/[email protected] worked for me. I had to delete the package-lock.json and run npm install again. Very odd

@Tautorn
Copy link

Tautorn commented Jul 27, 2020

@benjamn I just tried the latest version @apollo/[email protected] and @apollo/[email protected]. When I try execute the search below the query gets stuck in an infinite loop, making multiple network requests and never calling onComplete. If I comment out either the fetchPolicy or onCompleted it stops but the combination of both fetch fetchPolicy: 'no-cache' and onCompleted: data => {} cause the query to go into an infinite loop.

I tried replicating it in your sandbox but I can only replicate it with @apollo/[email protected].

const [search] = useLazyQuery(SearchQuery, {
    fetchPolicy: 'no-cache',
    onCompleted: data => {},
  })

const handleClick = () => { 
  search({
    variables: {
       query: 'search',
    }
  }) 
}

Hi there.

The cache is enabled, but I don't use fetchPolicy to get any result.

I'm using onCompleted after query. Before I updated to @apollo/[email protected] was necessary wrapper useQuery and useLazyQuery with useCallback.

But now working normally (after update version).

@zeeshanaligold
Copy link

const { loading, error } = useQuery(GET_USER, {
    onCompleted({ user }) {
      setAuth({ user, isAuthenticated: true })
    },
})

Still getting stuck with infinite loop of requests to api. it's happening only when i call setAuth to save state with context api.

@orrybaram
Copy link

@zeeshanaligold try adding a flag to check for the user in the onCompleted method. You may be accidentally retriggering the infinite loop yourself by setting state on each render (which cause it to re-render again)

onCompleted({ user }) {
  if (!authedUser) {
    setAuth({ user, isAuthenticated: true })
   }    
}

@Tautorn
Copy link

Tautorn commented Jul 28, 2020

const { loading, error } = useQuery(GET_USER, {
    onCompleted({ user }) {
      setAuth({ user, isAuthenticated: true })
    },
})

Still getting stuck with infinite loop of requests to api. it's happening only when i call setAuth to save state with context api.

@zeeshanaligold try with useCallback:

import { useCallback } from 'react'
...

const handleOnCompleted = useCallback(({ user }) => {
    setAuth({ user, isAuthenticated: true })
}, [])

const { loading, error } = useQuery(GET_USER, {
    onCompleted: handleOnCompleted
})

@zeeshanaligold
Copy link

zeeshanaligold commented Jul 28, 2020

@zeeshanaligold try adding a flag to check for the user in the onCompleted method. You may be accidentally retriggering the infinite loop yourself by setting state on each render (which cause it to re-render again)

onCompleted({ user }) {
  if (!authedUser) {
    setAuth({ user, isAuthenticated: true })
   }    
}

@orrybaram it's fix the infinite loop issue but still sending 2 requests.

One more issue when logout trigger & authedUser will set to false then again new request will go to graphql

@zeeshanaligold
Copy link

import { useCallback } from 'react'
...

const handleOnCompleted = useCallback(({ user }) => {
    setAuth({ user, isAuthenticated: true })
}, [])

const { loading, error } = useQuery(GET_USER, {
    onCompleted: handleOnCompleted
})

@Tautorn i did try it but still same issue.

@Tautorn
Copy link

Tautorn commented Jul 28, 2020

@zeeshanaligold Could you reproduction this issue in some tool like codesandbox? If possible I hope.

@FezVrasta
Copy link

Does 3.1.1 include this fix?

@GeorgeMarcusGG
Copy link

Not fixed on 3.1.1. It still only works by wrapping onCompleted and onError in useCallback.

@emjaksa
Copy link

emjaksa commented Jul 29, 2020

3.1.1 is working for me

@3nvi
Copy link
Author

3nvi commented Nov 26, 2020

Although we're about to release v3.2.0 with the changes that are currently on the release-3.2 branch, we haven't forgotten about this issue. We hope to include a fix in a 3.2.x (or a 3.3 beta) version soon.

@benjamn Given the fact that 3.3.0 launched without it, is there a chance we can get an update on the state of things? Is it something that's within your priorities or should we try and work around this for the time being.

As always, I appreciate you' re juggling a lot of things, so thanks in advance

@adrienharnay
Copy link

As always, I appreciate you' re juggling a lot of things, so thanks in advance

👍 on this, thanks for the work you're putting into this project!

@araujo-luis
Copy link

araujo-luis commented Jan 20, 2021

Any update on this? I'm getting this error on version

"@apollo/client": "^3.2.5",
"apollo": "^2.31.1",

I found an orthodox solution, implemented useEffect which acts as a completed validator:

const { loading, data, error } = useQuery( MY_QUERY, {
      variables: { id },
    },
  );

Then my useEffect:

useEffect(() => {
    if (!data) return;

    // your onCompleted logic here

}, [data]);

@nadaru-fr
Copy link

nadaru-fr commented Feb 4, 2021

Would be nice to have a proper onComplete fired on refetch even if it's from cache.

The most similar behavior I could come up with meanwhile was:

  useEffect(() => {
    if (query.networkStatus === NetworkStatus.ready) {
      ...on complete...
    }
  }, [query.networkStatus])

@kftsehk
Copy link

kftsehk commented Mar 20, 2021

adding that {loading} doesn't include the code to process onCompleted, and will not fire onCompleted when another invocation (possibly with different variables) is in progress, making it hard to be compatible with any infinite scroll library.

"react-apollo": "^3.1.5",

@brainkim brainkim self-assigned this Jun 22, 2021
@everdrone

This comment has been minimized.

@brainkim brainkim mentioned this issue Aug 13, 2021
14 tasks
@brainkim brainkim reopened this Aug 18, 2021
@brainkim
Copy link
Contributor

brainkim commented Nov 18, 2021

This should be fixed in 3.5, please try it.

@3nvi
Copy link
Author

3nvi commented Nov 18, 2021

@brainkim it's not. Unfortunately, I believe it's now worse than before, since onCompleted gets only triggered once for all lazy queries with network-only policy, regardless of the number of times they are called. This means that even if I perform multiple network requests, I'll still have only 1 onCompleted callback fired.

Please refer to this comment #6636 (comment) and its reproduction instructions to validate the bugs. I was able to validate them in [email protected].

Should we re-open this?

@brainkim brainkim reopened this Nov 18, 2021
@brainkim
Copy link
Contributor

brainkim commented Nov 18, 2021

Unfortunately, I believe it's now worse than before

No, I would disagree with that. As far as I can tell, the original issue was about React render-looping when there was some combination of fetch policy and onCompleted() passed to useQuery()/useLazyQuery(). I’ve run through the examples listed in this issue and have yet to see render-looping happen in 3.5. The other “bugs” listed throughout this issue are various expectations about when the onCompleted() callback should be called, but honestly I think this is underspecified by the documentation, which states:

A callback function that's called when your query successfully completes with zero errors (or if errorPolicy is ignore and partial data is returned).

I’ll sift through this monster thread again to see if I can codify the general expectations as unit tests. Thanks for the patience.

@sbsupper
Copy link

sbsupper commented Dec 7, 2021

This issue was not fixed for three years...

@mcsimps2
Copy link

@brainkim I'm new to using Apollo Client. My "blank slate" expectation of reading the onCompleted section of the useLazyQuery documentation was that it would be called every time the query was run rather than just the first time. If that wasn't the intention, then updating the docs would definitely save newcomers like myself a lot of digging through GitHub issues.

@hwillson hwillson added the 🔍 investigate Investigate further label May 31, 2022
@hwillson
Copy link
Member

hwillson commented Jun 6, 2022

We believe this is now fixed; if anyone is still encountering this issue in @apollo/client@latest, definitely let us know. Thanks!

@hwillson hwillson closed this as completed Jun 6, 2022
@3nvi
Copy link
Author

3nvi commented Jun 7, 2022

@hwillson indeed now the onCompelted is always fired, so this bug has been resolved. Thanks for that!

One of the things that still lingers is the fact that changing variables on a lazy query (that has previously been executed), fires it automatically.

Due to the nature of lazy querying, I'd expect that changing variables wouldn't automatically run a lazy query, but this may be by design. I personally consider it a bug (and part of this ticket), but let me know your thoughts.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔍 investigate Investigate further
Projects
None yet
Development

No branches or pull requests