apollographql / apollo-feature-requests

🧑‍🚀 Apollo Client Feature Requests | (no 🐛 please).
Other
130 stars 7 forks source link

useQuery with pollingInterval loading state not as expected #373

Open don-hover opened 2 years ago

don-hover commented 2 years ago

Intended outcome:

Polling has two general use cases:

  1. We want to poll in the background to get notified of an update to some external state. We either don't need to show a loading UI at all, or only care about showing a loading UI while the actual network poll request is happening.
  2. We initiate a long-running process on a remote system, and would like to like to poll to be notified of completion of the process, and show a loading UI during that time.

Actual outcome:

The useQuery state variables work well for case 1., but they don't accommodate case 2., even when making use of the notifyOnNetworkStatusChange option. For case 2. there's a logical concept of polling starting, and polling continuing until it is stopped, and that loading is occurring for the time between these two states. For case 2., we'd expect that loading would remain true until polling is explicitly stopped using stopPolling().

In addition to the network-level indication that a polling request is in process, there should also be a higher-level client-level indication that a polling query is in process. ApolloClient should know that polling is in-progress, if polling has been started and not stopped - and the ApolloClient API should be able to be interrogated for that logical state.

How to reproduce the issue:

Execute a useQuery hook with a pollingInterval option of something like 500 and with notifyonnetworkstatuschange: true. Set a timeout so that polling continues for 10 seconds and then stopPolling() is called. In the React component, add log statements like these:

console.log("Loading state:", loading);
console.log("Network status:", networkStatus);

Observe -

A series of log statements will be written to the console like this:

Loading state: true
Network status: 6
Loading state: false
Network status: 7
Loading state: true
Network status: 6
Loading state: false
Network status: 7
Loading state: true
Network status: 6
Loading state: false
Network status: 7

If we try to use these state values to control the display of a Loader component or to control page rendering, we'll see the page flickering between loading and rendering state. So, another indication of long-polling is needed.

Versions

System: OS: macOS 11.3 Binaries: Node: 15.6.0 - ~/.nvm/versions/node/v15.6.0/bin/node Yarn: 1.22.0 - ~/hover/ehi-frontend/node_modules/.bin/yarn npm: 7.4.0 - ~/.nvm/versions/node/v15.6.0/bin/npm Browsers: Chrome: 95.0.4638.69 Firefox: 94.0 Safari: 14.1 npmPackages: @apollo/client: ^3.3.21 => 3.3.21 @apollo/rover: ^0.4.0 => 0.4.0 apollo: ^2.33.9 => 2.33.9 apollo-client: ^2.6.10 => 2.6.10 mock-apollo-client: ^1.2.0 => 1.2.0 react-apollo-hooks: ^0.5.0 => 0.5.0

brainkim commented 2 years ago

Sorry to hear you’re having trouble. Can you try 3.4/3.5? I did an overhaul of polling around summer-time so these sorts of issues probably should be fixed in 3.4/3.5 🤞

don-hover commented 2 years ago

@brainkim Can you please provide some information about what may have changed in that overhaul? Is there anything specifically related to what I mentioned about the difference between network-level polling and logical polling state?

brainkim commented 2 years ago

3.4 and 3.5 dealt with misbehavior in the polling function. It seems like you’re asking about a query that polls for the duration of polling sequence, and knowing that this polling sequence is havppening. If that‘s what you want, then you can use the startPolling() and stopPolling() functions, along with another piece of state which indicates that you’re polling. Something like:

const [polling, setPolling] = useState(true);
const {data, loading, startPolling, stopPolling} = useQuery(MY_QUERY);
useEffect(() => {
  if (polling) {
    startPolling(3000);
  } else {
    stopPolling();
  }
}, [polling]);

// The polling variable can be unset in a long timeout, to achieve the use-case you want, and can be read to determine that we’re in a long poll.

I’m not really sure that notifyOnNetworkStatusChange will do what you want here.

don-hover commented 2 years ago

3.4 and 3.5 dealt with misbehavior in the polling function. It seems like you’re asking about a query that polls for the duration of polling sequence, and knowing that this polling sequence is havppening. If that‘s what you want, then you can use the startPolling() and stopPolling() functions, along with another piece of state which indicates that you’re polling. Something like:

const [polling, setPolling] = useState(true);
const {data, loading, startPolling, stopPolling} = useQuery(MY_QUERY);
useEffect(() => {
  if (polling) {
    startPolling(3000);
  } else {
    stopPolling();
  }
}, [polling]);

// The polling variable can be unset in a long timeout, to achieve the use-case you want, and can be read to determine that we’re in a long poll.

Yes, using a local React state is exactly what we've done to accomplish this. However, this seems like something that ApolloClient should handle.

I’m not really sure that notifyOnNetworkStatusChange will do what you want here.

No, it won't, because as I said that deals only with network-level state.

ApolloClient could easily incorporate this exact type of state-tracking, and from looking at the source code it nearly does already. There are internal functions/handlers that respond to the calls startPolling()/stopPolling(), and between each start and stop, there is a state that we're interested in knowing about - the fact that polling is in progress. This seems like a core state of the Client that should be exposed in its API.