Fix infinite loop caused by set state in onError / onCompleted props of Query

[chenesan]

Fix #2477 .
The infinite loop is because in #2190 we try to handle onError/onCompleted for local cache so we move the handler logic from updateCurrentData to componentDidUpdate. Once the upper component update its state in onError/onCompleted handler, it will cause Query update and in componentDidUpdate it would call onError/onCompleted again, so the infinite loop happens.

In this PR we move the onError/onCompleted handle logic back to updateCurrentData, and in componentDidUpdate check if the query and variables are as same as prevProps (I'm not sure if it's safe, or we should consider more other props here). if it's different then we check if the currentResult is loaded / error (cached) to call handler. If the update is caused by fetching success/error, the props will be the same so it will not call onError/onComplete in componentDidUpdate.

[apollo-cla]

@chenesan: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[chenesan]

@hwillson the "github account" field in MCA website seems broken, I didnt' find any button or input to make me sign in github.

[hwillson]

Thanks for the heads up @chenesan - we'll take a look.

[hwillson]

@chenesan The CLA app should be working again. Thanks!

[chenesan]

@hwillson thanks, that works!

[stnwk]

Thank you for tackling this issue, would really love to see this PR merged 😍What's the current state? @hwillson @chenesan

[chenesan]

The fix is done and waiting for review :-)

[chenesan]

@benjamn @hwillson Is there anything I can do to make progress for this PR?

[stnwk]

@benjamn @hwillson friendly ping

[stnwk]

Hello @peggyrayzis :)

This PR is sitting ready for over 3 weeks now without any sort of response from the apollographql team. I feel like this both unfair to the community and the contributor @chenesan.

I figured, as the Engineering Manager of apollographql you might want to take a look and improve the situation :)

Thank you very much.

[jortizsao]

any news on the code review?

[hwillson]

Thanks very much for working on this @chenesan!

[hwillson]

I'd like to get some feedback on these changes before merging them. If anyone is interested in helping test these changes out, please install react-apollo@next (2.5.2-rc.0) and let me know if everything looks like it's working properly. Thanks!

[shadrech]

@hwillson When will this be released? I have react-apollo v2.5.2 and this fix doesn't seem to be present. Really needed as I'm also coming across onError infinite loops

[adrienharnay]

@shadrech This has been released in 2.5.3

[shadrech]

👍🏾 @adrienharnay

[sekoyo]

Is this fix in @apollo/react-hooks? I get this issue with 3.0.0

[adrienharnay]

It seems like there is no issue with @apollo/react-hooks: https://codesandbox.io/s/goofy-robinson-rfyjr

[sekoyo]

@adrienharnay Ah yes but I was talking about in useQuery:

      fetchPolicy: "cache-and-network",
      onCompleted: () => {
        setCount(prevCount => prevCount + 1);
      }

Maybe it's an anti-pattern. I ended up resolving it by using React's useMemo to compute a value once data changed.

[adrienharnay]

Ah, indeed. Can confirm the bug is still occuring, but... Is it even a bug? Is it not normal that, when a Query/useQuery is re-rendered, the query is performed again (thus, changing state in onCompleted would result in an infinite loop, just like changing state in useEffect, but as you said you can guard against this by using useMemo)?

It seems counter-intuitive to me for Apollo to handle this. A user might wonder why onCompleted/onError is called once, but not called on subsequent queries.

[kylehoover]

But it doesn't follow the same behavior as the Query component. I'm migrating to hooks right now, and I'm having to deal with the issue of onCompleted being called every render, where it wasn't an issue before. I always saw onCompleted being something that would only be called when new data is fetched. Also, as pointed out here #3353 (comment), passing an empty variables object in the useQuery options stops this behavior, which makes it seem more like a bug.