Make ObservableQuery.lastResult keep track of errors

[jasonpaulos]

apollographql/react-apollo#2926 demonstrates a case where notifyOnNetworkStatusChange fails to report a change. This issue happens because networkStatusChanged is incorrectly false in the logic here

Since ObervableQuery.lastResult is not currently updated when an error occurs, it will continue to have networkStatus = 4 after an error occurs when fetching a query. When a query begins loading again after just experiencing an error, it also has networkStatus = 4, so the above code chooses not to notify the user that the query is loading.

I fixed this by having ObservableQuery take notice when an error occurs and update lastResult the same way that the query result is updated by QueryStore.markQueryError. Ideally the actual query result would be used here, but since the observable is only passed the error, this can't happen (maybe with async iterators it can 👀).

This fix addresses the following comments in react-apollo's test suite as well, which can be reincorporated if this change is merged into apollo-client.

• Query.test.tsx
• errors.test.tsx

---

Fixes apollographql/react-apollo#2926
Fixes apollographql/react-apollo#321

[hwillson]

Wow, great catch @jasonpaulos! Do you think we need to be worried about backwards compatibility with this? It's always tricky trying to figure out if a bug fix like this can be considered breaking, since people have gotten used to working around it being broken 😬. For example, since this means we can fix https://github.com/apollographql/react-apollo/blob/a1e0b6d41158cfcf447dfe69474e79257d5c26b8/test/client/Query.test.tsx#L1464..L1469, it also means we're triggering an extra loading render that people might not have been expecting. In theory this shouldn't be an issue, but we've run into trouble with this situation before. @benjamn I'd love to hear your thoughts as well. I'd love to get this in!

A few small additional comments about the PR:

• Would you mind adding a small note about this change in the CHANGELOG?
• Should we add an additional test for this to make sure we don't regress in the future? Once we fix the tests in RA, we'll have that safety check, but we will probably want to test this in AC as well.

Thanks again!

[jasonpaulos]

@hwillson Excellent points. I've added a regression test and updated the changelog to reflect the proposed changes.

It may be the case that some developer code relies on the library's behavior as is, and I'm not sure how detrimental a fix like this would be in those cases. Perhaps @benjamn has an idea of this? However, I believe in most cases this fix will cause networkStatusChanged to behave more in line with what users of this library expect.

[hwillson]

Sorry for the long delay here @jasonpaulos - this looks great! Thanks very much!