Use fetchMore network status

[calebmer]

Closes #851

This PR makes sure that the fetchMore network status is set appropriately in Redux when fetchMore is called from ObservableQuery. Because fetchMore spawns a new query with a different id the implementation of this network status is a little tricky. It feels like the best way to do it is adding the query id we may be fetching more for to our Redux actions which will change the network status on the correct query. Unfortunately, there is no easy, clean, way to do that.

This PR also cleans up error handling for fetchMore so if an error is thrown in a query spawned by fetchMore that error will be correctly propagated back to the parent query.

I contemplated keeping the information required to compute a fetchMore network status out of the store until I remembered that if I did that then there would be no way to notify observers that the computation changed 😣. So into Redux it must go!

This is an item on our roadmap to 1.0.

• If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change

[helfer]

Looks good to me, apart from the fact that I think lower network statuses should take precedence over fetchMore (maybe fetchMore and subscriptions is where the network status model starts to break down). I share your concern that this will be a bit hard to maintain, but we do need some solution for fetchMore status and I think this will do.

[helfer]

The way networkStatus was planned is that lower numbers should have precedence. For example, it's probably more important if the query is refetching than whether a fetchMore is happening.

[calebmer]

We could implement something like that. The reason I didn’t do it is that in the current implementation networkStatus gets clobbered no matter what it is currently set to:

apollo-client/src/queries/store.ts

Lines 81 to 93 in 4bf5e67

	// TODO break this out into a separate function
	let newNetworkStatus = NetworkStatus.loading;
	if (isSetVariables) {
	newNetworkStatus = NetworkStatus.setVariables;
	} else if (action.isPoll) {
	newNetworkStatus = NetworkStatus.poll;
	} else if (action.isRefetch) {
	newNetworkStatus = NetworkStatus.refetch;
	// TODO: can we determine setVariables here if it's a refetch and the variables have changed?
	} else if (action.isPoll) {
	newNetworkStatus = NetworkStatus.poll;
	}

Also, if we had multiple queries running at once where one was a refetch and another was a fetchMore and the fetchMore resolved first it would set the networkStatus to ready even if fetchMore was never set because there was a refetch running and the current state would be incorrect because the refetch is still running. Move the execution order and type around and you get similar problems.

Unless I’m missing something there are all kinds of places where networkStatus has the potential for weird edge cases in concurrent situations. So the question is whether we should refactor networkStatus for a complete and correct implementation, continue with the current approach for now, or go somewhere in the middle which may mean not replacing with a fetchMore if a refetch is active for example.

Thoughts?

[helfer]

🤔 Those lines you referenced look a bit strange to me, even though I wrote them. Apart from everything getting clobbered, action.isPoll gets checked twice.

Apart from fetchMore, the other network statuses could be kept relatively clean. They're all happening on the same query, so we reject any result that doesn't come from the most recent query. We could do something similar for fetchMore by ignoring the result of a fetchMore if the query's status isn't fetchMore.

As was pointed out in another issue, it isn't even clear what it means to call fetchMore when you're polling or refetching, because the result would get clobbered anyway. Because of that I think the current implementation is fine, and we can stop thinking about it for now. It would be very nice to have an implementation where you can have multiple actions on a query happening at the same time and they all get sequenced in a meaningful way, but that time is not now imho.

[helfer]

Why are there two events for the fetchMore result? Shouldn't the network status update to ready at the same time as the fetchMore comments are added?

[calebmer]

There are two actions here which affect the relevant state which get dispatched here and so we get two events on our ObservableQuery:

1. The APOLLO_QUERY_RESULT from the QueryManager#fetchQuery in ObservableQuery#fetchMore. This causes the state to update so that the network status switches to ready even though the update has not yet been applied.
2. The APOLLO_UPDATE_QUERY_RESULT from the ObservableQuery#updateQuery call at the end of ObservableQuery#fetchMore that actually updates the store resulting in the second event actually getting the +10 comments.

These two actions are probably dispatched in the same turn of the event loop, so it may be interesting to pursue debouncing our observers by one tick. This would also solve some problems with errors getting swallowed, but would likely break a lot of tests.

[helfer]

Oh, interesting. I didn't remember that updateQueries fires a separate action. Debouncing observers is an interesting idea, but not something we should do now.

[helfer]

to -> too

[calebmer]

👍

[helfer]

Looks good to me, let's merge it! :)