apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.35k stars 2.66k forks source link

Using useQuery with pollInterval triggers onCompleted only once #5531

Open kocur4d opened 4 years ago

kocur4d commented 4 years ago

Intended outcome:

I would expect the onCompleted callback to be fired after every poll.

Actual outcome:

onCompleted is being fired only once. First time the query is made.

How to reproduce the issue:

const { data, stopPolling } = useQuery(QUERY, {
  variables: {...},
  fetchPolicy: 'network-only',
  pollInterval: 1000,
  onCompleted: () => console.log('called')
})

Versions

npmPackages: @apollo/react-common: ^3.0.1 => 3.0.1 @apollo/react-hooks: ^3.0.1 => 3.0.1 apollo-cache-inmemory: ^1.3.5 => 1.3.11 apollo-client: ^2.6.4 => 2.6.4 apollo-link: ^1.2.3 => 1.2.4 apollo-link-context: ^1.0.10 => 1.0.10 apollo-link-error: ^1.1.1 => 1.1.2 apollo-link-http: ^1.5.5 => 1.5.7 apollo-link-logger: ^1.2.3 => 1.2.3 apollo-server-koa: ^2.1.0 => 2.2.4 apollo-utilities: ^1.3.2 => 1.3.2 react-apollo: ^2.2.4 => 2.3.2

ajhool commented 4 years ago

Have you checked the network traffic? I believe that it does not actually poll, as opposed to executing onComplete only on the first query.

kocur4d commented 4 years ago

@ajhool I have just tested it again and I can confirm that the query is being called multiple times. I can see it in the network traffic every second, but the onCompleted is being called only once.

jaredmdobson commented 4 years ago

Yeah i'm seeing the same thing.

        "apollo-cache": "^1.3.2",
        "apollo-cache-inmemory": "^1.6.3",
        "apollo-client": "^2.6.4",
        "apollo-link": "^1.0.6",
        "apollo-link-error": "^1.0.3",
        "apollo-link-http": "^1.3.1",
        "graphql-tag": "^2.4.2",
        "ts-invariant": "^0.4.0",
        "tslib": "^1.9.3"
ScripterSugar commented 4 years ago

+1 Having same problem here. onCompleted will be fired only once at the first fetch.

maddog986 commented 4 years ago

+1 Im also seeing the same issue

mamousavi commented 4 years ago

Setting notifyOnNetworkStatusChange to true solved the issue in my case.

bhishp commented 4 years ago

This could be the culprit?

https://github.com/apollographql/apollo-client/blob/6bc9fdcfc064bd60533794f0ef5aeed45f7ad537/src/react/data/QueryData.ts#L433

It seems onCompleted only runs when data has changed, so if you are polling and no data changes occur then it will not fire.

Ideally there could be an extra prop to always call. Or maybe an alternative function prop, onCompletedAlways?

bhishp commented 4 years ago

A workaround for our case was to add fetchPolicy: 'no-cache', to the query options. Thus:

const { data, stopPolling } = useQuery(QUERY, {
  variables: {...},
  fetchPolicy: 'network-only',
  pollInterval: 1000,
  onCompleted: () => console.log('called'),
  fetchPolicy: 'no-cache',
})

Obviously this means you will bypass the client-side cache but it will ensure the completion hook is triggered every time.

GollyJer commented 4 years ago

We've tried the workarounds here but none work.

We don't see onCompleted firing at all with @apollo/client 3.0.0-beta.37. Even the first call to the query doesn't print to the console.

  const messagesQuery = useQuery(GET_CHAT_MESSAGES_BY_GROUP_ID, {
    variables: { chatGroupId },
    pollInterval: 1000,
    onCompleted: () => console.log('If this worked no useEffect needed. 😕'),
  });
jcelliott commented 4 years ago

I'm running into this issue as well. Even with fetchPolicy: 'no-cache' the onCompleted handler is only being called once.

marcdavi-es commented 4 years ago

The same is true for onError. It's called for the first error but is not called if later poll attempts have errors. The workarounds do not help me.

3nvi commented 4 years ago

Can confirm as well

andreasonny83 commented 4 years ago

Same behaviour here

dominik-myszkowski commented 4 years ago

Guys, set the fetchPolicy: 'network-only', it should work then, I had the same problem. And better switch to: useLazyQuery instead of polling if it is possible.

3nvi commented 4 years ago

Guys, set the fetchPolicy: 'network-only', it should work then, I had the same problem. And better switch to: useLazyQuery instead of polling if it is possible.

Erm sorry but that's subjective. I need to store it in the cache since it's very expensive for me to re-fetch it. I need polling since I'm using it for an async server operation that has the results ready in between 30secs and 2 minutes, thus I need to continuously "check" if they are ready for serving

andreasonny83 commented 4 years ago

Also, @dominik-myszkowski , setting fetchPolicy: 'network-only' only triggers the onCompleted once. I can only get it working by setting notifyOnNetworkStatusChange: true

bhishp commented 4 years ago

This could be the culprit?

https://github.com/apollographql/apollo-client/blob/6bc9fdcfc064bd60533794f0ef5aeed45f7ad537/src/react/data/QueryData.ts#L433

It seems onCompleted only runs when data has changed, so if you are polling and no data changes occur then it will not fire.

Ideally there could be an extra prop to always call. Or maybe an alternative function prop, onCompletedAlways?

Are any of you falling in to this conditional? Is it not firing when no data has changed?

andreasonny83 commented 4 years ago

@bhishp I would really love the onCompeted event only to run on data changed. however even when the data changes, I cannot see the onCompeted method triggered

krishnaku commented 4 years ago

Setting notifyOnNetworkStatusChange to true solved the issue in my case.

This worked for me. Setting 'network-only' did not.

3nvi commented 4 years ago

@hwillson any updates on that with the release of 3.0?

martdavidson commented 4 years ago

@andreasonny83 I'm seeing the same, the data changed but my onComplete didn't fire. notifyOnNetworkStatusChange fixed the issue for me. Still happening even in version 3.1.0-pre.1

jure commented 4 years ago

The issue with setting notifyOnNetworkStatusChange is that it will rerender for every poll interval (as documented). This is probably not what you want and you'd want to rerender only when the data changes.

martdavidson commented 4 years ago

@jure Yes, but as @andreasonny83 mentioned and I've also confirmed, it isn't firing when the data changes.

jure commented 4 years ago

Right, absolutely, it should! That's the bug. I've commented merely to point out that setting notifyOnNetworkStatusChange isn't a workable workaround in many situations and that folks should be aware of that drawback before applying it willy nilly, as it causes the whole tree below the hook to rerender on every interval.

jure commented 4 years ago

For what it's worth, I've sort of resolved the issue caused by this workaround for the time being by chucking the polling useQuery into a dead end of the tree, so the rerendering isn't annoying. In there I then use makeVar which is then used in the typePolicies of the InMemoryCache, like this:

  {
    cache: new InMemoryCache({
      typePolicies: {
        Manuscript: {
          fields: {
            _currentRoles: {
              read(existing, { cache, args, readField }) {
                const currentRoles = currentRolesVar()
              },
            },
          },
        },
      },
    }

It's quite the detour, but it works, so hopefully it's useful for someone else too.

DogAndHerDude commented 3 years ago

Yup, can confirm. Still happening in 3.3.6. I resolved the issue with notifyOnNetworkStatusChange, though it's like using a baseball bat to clean the dishes.

The only other alternative I can come up with is using a useEffect, and refetch.

doflo-dfa commented 3 years ago

This has proven to be the most reliable combination for us. note the separate user queries and the manual starting and stoping of polling. this bypasses a second bug in which stop polling does not reliably work with pollinterval

  const [completed,setCompleted] = useState(false);

  const { data: versionData, loading: versionLoading, stopPolling, startPolling } = useQuery(QUERY, {
    variables: {
      id: id,
    },
  })

useEffect(() => {
    if (versionData) {
        ...
    }, [versionData])

useEffect(() => {
    // versionRefetch()
    if (!completed) {
      startPolling(4000)
    } else {
      stopPolling()
    }
    return () => {
      stopPolling()
    }
  }, [stopPolling, startPolling, completed])
dylanwulf commented 3 years ago

I created a runnable reproduction of this issue (use branch polling-oncompleted-not-called): https://github.com/dylanwulf/react-apollo-error-template/tree/polling-oncompleted-not-called

hwillson commented 2 years ago

This should now be resolved in @apollo/client@3.5.0. Let us know if you notice any issues. Thanks!

3nvi commented 2 years ago

I don't think this is fixed @hwillson

Here's a codesandbox repro. Making notifyOnNetworkStatusChange: true fixes it (as it always did), but when it's false, the original issue still exists.

Should this be re-opened?

d4v1d41 commented 2 years ago

I don't think this is fixed @hwillson

Here's a codesandbox repro. Making notifyOnNetworkStatusChange: true fixes it (as it always did), but when it's false, the original issue still exists.

Should this be re-opened?

Same, I just tried with version 3.5.6, still not working. notifyOnNetworkStatusChange: true does the trick though.

adamszeptycki commented 2 years ago

I have the same problem with latest version (for me it happens both on useMutation and useQuery - two different use cases)

tonyjmnz commented 2 years ago

This issue persists in 3.5.8.

It's baffling how issues like this can go multiple years without being addressed in such a widely used library... And this is not the first one I've seen.

I suggest reopening this issue.

eduardogch commented 2 years ago

I confirmed that the issue persists in the current latest version 3.5.8 even with the option notifyOnNetworkStatusChange: true

My work around to called onCompleted and solved this issue. It was to implement a timer or setInterval in a useEffect and removed the pollInterval option from the query. Example:

// From a useQuery
const { data, refetch } = useQuery(QUERY, {
  variables: {...},
  onCompleted: () => console.log('called'),
})

// From a useLazyQuery
const [getData, { error, loading }] = useLazyQuery(QUERY, {
  variables: {...},
  onCompleted: () => console.log('called'),
})
useEffect(() => {
    const interval = setInterval(() => {
      refetch() // Refetch useQuery
      getData(); // Called useLazyQuery
    }, 5000);
    return () => clearInterval(interval);
  }, []);
andreasonny83 commented 2 years ago

@akikoskine, @brainkim, can we reopen this issue?

squarewave24 commented 2 years ago

same issue, in addition to onCompleted, my react table is not reflecting changes even though i see the calls in network. notifyOnNetworkStatusChange is making everything re-render on each poll so that's not really a solution i can use.

imorad87 commented 2 years ago

My work around:

const { loading, error, data, startPolling, stopPolling } = useQuery(GET_ALL_CAMPAIGNS)

useEffect(() => { startPolling(1000); return () => stopPolling(); }, []);

alessbell commented 1 year ago

Hi all - after taking a look at @3nvi's codesandbox (thanks for providing that!), onCompleted only fires once because the data never changes. So while the network requests are firing in the background, the cache remains in the same state as after the initial fetch. In this case, we do not fire onCompleted unless notifyOnNetworkStatusChange is set to true (but as others have noted, this also updates the loading state).

However, if the selection set observed by your query is being updated in your cache, onCompleted should be firing (even without notifyOnNetworkStatusChange: true), so any reproductions to the contrary would be helpful.

If anyone has any feedback on pollInterval + onCompleted, feel free to send them my way! I'm taking a look at this issue as part of https://github.com/apollographql/apollo-client/pull/10229

mogelbrod commented 1 year ago

Glad to hear that you're looking into this @alessbell!

Not being able to reliably subscribe/listen for (refetches/polling) responses via onCompleted without also setting notifyOnNetworkStatusChange: true has lead to our codebase having to wrap the returned query.refetch() function (and any other functions that could trigger a new request) with something like this to attempt to intercept requests that do not update the cache:

function useQueryVariation(...) {
  const context = React.useRef()
  context.current = {
    onCompleted(data) {...}
  }

  const query = useQuery(..., {
    onCompleted: context.current.onCompleted,
  })

  return {
    ...query,
    refetch: React.useCallback(() => {
        return query.refetch().then(result => {
          context.current.onCompleted(result.data)
          return result
        })
      }, [query.refetch])
  }
}

To me this doesn't feel like a maintainable solution (the workaround doesn't capture refetches triggered in other ways) but rather like trying to reverse an implementation detail.

The name onCompleted (as well as the docs) suggest that it would be triggered whenever the hook completes (data is retrieved?), whereas the current behaviour would be a better fit for a onUpdated callback or similar. Perhaps there's an argument for having both, if the currently implemented behaviour is sometimes desired?

tathagat2000 commented 11 months ago

@alessbell @hwillson @ajhool Please consider this useCase.

fetchPolicy is cache-and-network. Query goes and data is present in cache. onCompleted is called.

Simultaneously Network call goes (Because fetchPolicy is cache-and-network). In the network call, data has changed which causes cache to update BUT, onCompleted will never be called with this new data. Isn't this not a valid usecase where onCompleted should have been called?

Additional: onCompleted won't be called the second time because of this check -> previousResult?.networkStatus !== result.networkStatus. networkStatus doesn't change in our above useCase when cache is revalidated with network request.

Please check

ColonelThirtyTwo commented 6 months ago

Bumping this because I ran into it. I was using pollInterval to check on a job, and set it to zero once the job is done (since there won't be any updates afterwards). I have fetchPolicy: "no-cache" and the data I fetch definitely changes, but onCompleted only runs once. As other have mentioned, notifyOnNetworkStatusChange: true causes onCompleted to rerun.

The fact that onCompleted runs only once is unintuitive and undesireable. The fact that its fixed by setting a seemingly unrelated option notifyOnNetworkStatusChange changes the behavior is very unintuitive.

Haegin commented 6 months ago

I'm on an old version of Apollo Client so some of this may not be relevant to the current code, but when running into this I also found that when using polling if I added a useEffect to monitor the returned data from the query that wasn't being run even if the data in the query was changing. I eventually got it working by manually implementing polling with setInterval and refetch, using a useEffect to compare the results. Posting here in case that helps anyone else and in case the issue is more generally related to polling, not just to the onCompleted callback.