Fix polling when working with the skip option

[brainkim]

Checklist:

• If this PR is a new feature, please 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

Potentially Fixes:

• setting skip back to false doesn't restart polling #6982
• query with pollInterval does not skip once polling has started #7552
• useQuery sometimes doesn't start polling when skip is set to true #7667

Not really sure how to describe this PR. IMO this bug fix should have taken on the order of hours, but ended up taking up 2.5 days, and I don’t have a perfect handle on why the current logic works.

What made this take so long? I don’t mean this as a litany of excuse but more as a sort of mini-post-mortem about code quality in Apollo Client.

The Apollo Client Options Grinder

Options are propagated, cached and mutated across at least three classes (QueryData, ObservableQuery, Reobserver). The specific reason for polling not working when skipping is that when skipping we mutate the pollInterval option, but when skipping we don’t pass the new options through the grinder so options.pollInterval stayed at 0. All I’m doing is making sure options continue to be passed through the grinder so that even though we mutate the pollInterval option, it gets updated when skip goes from true to false.

Relatedly, these options are checked as deep equal, except we let multiple options which aren’t related to any of the classes fall through without pruning them. Things like onCompleted/onError callbacks, which without useCallback hooks would not be referential equal across renders, and in some cases, client instances seem to leak into options as well. We should select for exactly which options we need before passing them along. I believe we are overly reliant on deep equality checks throughout the codebase and making changes often involves finding ways to disable a couple deep equality checks somewhere.

My suggestions are to 1. normalize (as in deduplicate) options so that they are only stored in one class and 2. Stop mutating options as a way to affect program state. Besides pollInterval stuff, we also mutate fetchPolicy options to do various things including refetching. As far as normalization goes, I think ObservableQuery would be the best place to put this stuff. In my humble opinion, I do not think the Reobserver class needs to exist, but that’s just me being me. The worst part is that Reobserver.setOptions() is what kicks off network requests, which to me seems very non-ideal.

Unit Tests

When tests fail, output is noisy, because the assertion errors are logged to the console for some reason. Additionally, one thing I’ve discovered is that in our React specific tests, the assertion you see is typically not the assertion which first failed, which leads to confusing errors where you think you’re failing because of a specific render, but a previous render is actually failing as well.

nextFetchPolicy

I am familiar with the reasons for why nextFetchPolicy was implemented, I still disagree about its existence. The problem after I fixed polling was that there was a failing unit test. Again, we mutate an options object, in QueryManager of all places to implement nextFetchPolicy behavior, and I’m not sure this is a robust way to do it.

As far as I can tell, nextFetchPolicy just means the fetchPolicy which should be used for non-initial fetches. I’m not really sure why allowing skipped options to propagate was changing the behavior of nextFetchPolicy, but the solution I chose was to fix nextFetchPolicy in QueryData. I am very spooked by the fact that this behavior is only tested in a higher-order component test.

This is almost certainly a band-aid, but if we’re doing a rewrite of the React part of Apollo Client, I don’t think it’s wise to do a more involved refactoring.

[brainkim]

Confirming this works live in https://github.com/apollographql/react-apollo-error-template/tree/polling-repro

[hwillson]

Thanks very much for addressing this @brainkim! We're all definitely looking forward to a React layer rewrite, and continuing to evolve the codebase. 👍 There is a lot of history in the AC source, some of which has served us quite well, some of which has served us less well. We have a good amount of work (more like fun) ahead of us to evolve things, but we'll get there. 🙂

One quick note - would you mind adding a small note about this fix in the CHANGELOG?

[benjamn]

These changes makes sense to me, though I agree the mutation of options is a source of complexity. There's more background here than meets the eye, but that's beside the point.

I'm going to push a commit that consolidates the options.nextFetchPolicy update logic into the ObservableQuery.ts module, so at least that logic will be defined only once.

I'd also like to retarget this PR against release-3.4 instead of main, so we can test it more easily, though of course we can back-port the changes to main later, if necessary. I can do that when I push my commit (it's a painless rebase, thankfully).