Fix component stuck in loading state for network-only fetch policy

[jasonpaulos]

Due to inconsistencies between Query.lastResult and the actual previous result passed to the child component for rendering, sometimes the Query component would suppress a render that contained necessary data to bring a component out of a loading state. Concretely, this happens when using a network-only fetch policy and the variables of a query changes but the result remains the same.

See #2899 for a complete description of the issue.

[apollo-cla]

@jasonpaulos: 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/

[hwillson]

Thanks very much for working on this @jasonpaulos - this has been a long standing React Apollo issue, so it's great to see traction here!

Looking this over, I think now might be a good time to take this solution further. The reason why your lastRenderIsDifferent approach works, is unfortunately because of a deep rooted issue that has been part of React Apollo for a long time. It's not necessarily a bug; it's more of a problem with the current architectural approach. I'll try to explain this issue below.

In the current Query component, after the initial creation and first render of the component (where things are in put into a loading state), we check for future data updates via Apollo Client by setting up a subscription on an Apollo Client ObservableQuery instance. This is the code that happens in the Query component's startQuerySubscription method. This idea here is that as soon as a data update is received through the ObservableQuery subscription, we can then force the Query component to re-render.

When we force a component to re-render in startQuerySubscription, after new subscription data is received, we end up calling the getQueryResult method. This is the method that takes the new data from Apollo Client, cleans it up a bit, then returns this as the data that is used in the render prop function. So getQueryResult ultimately gives us the data that is used in each render.

This is where the crux of the issue really comes in. startQuerySubscription leverages ObservableQuery to watch for new data, then when new data is found, triggers a re-render by force updating the component. The call for that re-render / forced update happens within the same tick of the event loop as the call to getQueryResult to get and return the resulting data. getQueryResult gets the current data by calling into Apollo Client's ObservableQuery.currentResult() function, which in theory should have the most recent data available. But, startQuerySubscription has set up its own subscription on the ObservableQuery instance, so when it receives new data, and eventually calls down into getQueryResult (in the same tick), ObservableQuery.currentResult() won't return that new data yet - it will still return the previous data as it hasn't had a chance to refresh yet, using its own internals.

The real problem here is the fact that we're asking for data from an ObservableQuery instance in 2 places: 1) in startQuerySubscription via the subscription we setup, and 2) in getQueryResult via a call to ObservableQuery.currentResult(). We're then using the data received in startQuerySubscription to decide when to call getQueryResult, expecting the same new data to be used, but getQueryResult is then using its own version of the data which is out of date.

This is definitely a bit convoluted to explain, but to make matters worse here we're leveraging lastResult to essentially work around the fact that the two places we load Apollo Client data from aren't always the same (and to block unnecessary renders). Your changes to use lastRenderIsDifferent, along with lastResult handling, do work around this issue, but the core problem still remains.

If we take a step back here a bit and think this through, we should be able to find a way to always make sure that if our ObservableQuery subscription says new data is available, and we're going to force a render based on that new data being available, that the render itself will always use the exact same data (taking into consideration a stored last render result of some kind, to decide of we should trigger a new render - ie, only if data has changed). I don't think Apollo Client is the problem here; the ObservableQuery API seems to be doing what it should. I think instead the way we've set ourselves up to watch for data changes in Apollo Client, then trigger a render based on that data, needs to be streamlined as our timing is off.

I'll setup a meeting to discuss this further (cc @benjamn). You're definitely headed in the right direction on this, and it would be amazing to finally nail this problem down once and for all. Thanks again @jasonpaulos!

[hwillson]

I've added changes in 5839fb9 to help address the issues raised in #3126 (comment). The current result to be used/displayed is now aligned across the entire component, and render tracking/control is now managed via a single instance variable called lastRenderedResult (which is always the last truly rendered result).

[benjamn]

Happy we took this opportunity to simplify Query state management! I left a few small comments, but I think this is pretty much ready.