Refactor QueryManager to make better use of observables and enforce fetchPolicy more reliably #6221

These tests were swallowing "No more mocked responses..." errors because of the missing call to setOnError. After I fixed that, the tests began consistently failing because they made too many requests of the mock link chain, so I reduced the expected number of requests, while mostly preserving the original spirit of the tests.

Though this API is not perfectly aligned with the RxJS multicast operator (no explicit Subject, for example), the spirit is definitely the same, and "multiplex" was mistaken terminology: https://rxjs.dev/api/operators/multicast

If we move polling logic from QueryManager into ObservableQuery, then we will always have an ObservableQuery available to consume results when we are polling, which should eliminate the non-ObservableQuery branch of this condition.

I'm not completely happy with this, but it seems to keep some existing tests passing.

Since it now returns an array of observables, rather than a combined observable, the 'o' in fqoHelper is no longer strictly warranted.

QueryManager#fetchQueryObservable now manages this logic (see mightUseNetwork and related code), so we don't need to do as much work in shouldNotify.

ObservableQuery#isTornDown is not a public property, so testing its value by abusing the type system was always a bit of an overreach.

TODO These changes might not be necessary if we were properly updating/delivering loading states for the original query for which we're fetching more results. The tests that I removed entirely were duplicates of other tests that I fixed/adjusted.

The use of setTimeout in tests is often an invitation to race conditions and timing-sensitive outcomes. This raciness became particularly problematic thanks to changes (between Node 10 and Node 12) in the timing of Promise callbacks with respect to setTimeout callbacks. These changes also leave all tests passing when cherry-picked onto master, so I'm confident I am *not* changing the tests just to fix my PR #6221, at the expense of backwards compatibility.

@hwillson

Now that the ObservableQuery consumes results exclusively through QueryManager#fetchQueryObservable, we can rely on the addExportedVariables call that happens there, and avoid potentially re-triggering a network request while consuming network results. @hwillson I was a bit surprised this change didn't cause any tests to fail, but several tests did fail when I cherry-picked it onto master, so I'm optimistic this addExportedVariables logic is genuinely redundant after my refactoring PR #6221.

@danReynolds

Thanks to @danReynolds for catching this. Co-authored-by: Dan Reynolds <me@danreynolds.ca>

#6221 (comment)

Besides addressing an important TODO, the privatization of the cancel method also resolves #6221 (comment)

#6221 (comment)

This functionality was introduced in PR #6058 (beta.43), and was temporarily removed by my refactoring PR #6221. I believe these warnings can be improved, especially since they are pretty noisy right now, but I wanted to preserve existing functionality before trying to figure out a better way to report missing fields. Any/all ideas and feedback welcome.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor QueryManager to make better use of observables and enforce fetchPolicy more reliably #6221

Refactor QueryManager to make better use of observables and enforce fetchPolicy more reliably #6221

Commits on May 7, 2020