Ensure remounted components properly adhere to fetchPolicy and nextFetchPolicy

[jerelmiller]

Fixes #10222

This fix ensures the fetchPolicy and nextFetchPolicy is properly adhered to when a component unmounts, then remounts. In v3.6.8 and below, a fetchPolicy of network-only and nextFetchPolicy of cache-first would properly return the loading state when remounted. In v3.6.9 and above, a remounted component would return cached data too early due to a change in #9823 that synchronously updated the fetchPolicy as soon as the initial query was kicked off.

Internally, we were using observableQuery.getCurrentResult() in our onNext function passed to the subscription to get the result of the query, rather than relying on the argument passed to onNext. This meant in some cases where onNext was triggered, we were calling observableQuery.getCurrentResult() with a fetch policy that had already been changed to the nextFetchPolicy, therefore we were returning cached data too early (in other words, the network-only result hadn't yet finished loading).

I opted to patch useQuery with some of the behavior in observableQuery.getCurrentResult() to try and limit the blast radius of the changes. The patch really should be a part of QueryManager, but I didn't know how how many people rely on the current behavior in how results are returned (which differ from observableQuery.getCurrentResult().

[jerelmiller]

This condition is already checked on line 264. To reduce noise, I went ahead and removed this since we already know its true.

[jerelmiller]

The more I work with this change, the less confident I feel in the solution. While I believe that using the argument passed to onNext is the right value we should be using (since it should technically be correct), there have been a lot of edge cases popping up with differences in data between the argument and obserableQuery.getCurrentResult(). It seems there is more done under the hood for getCurrentResult than meets the eye.

Given how much has been affected by this seemingly small change, it might be a signal that releasing an update such as this could have wide-ranging negative impacts. We can hope our tests catch 99% of them, but I'm not so sure. Something I've noticed while working on this fix is that many of our tests do not check for networkStatus. I've seen a case or two where the network status is completely different between the argument and the getCurrentResult() call, but the test doesn't seem to be catching it. I've only noticed the difference because a test might be failing for other reasons.

This change is what caused the unmount/remount bug, but I think @benjamn's reasoning for this change is sound and will help prevent other types of bugs. It feels a bit like a game of whack-a-mole at this point where fixing one thing seems to cause something else.

At this point, I feel I might need to consider a different approach. Ultimately I think we should highly consider fixing the root issue where we are unable to get the correct data through the onNext argument, but this fix feels a bit too costly right now.

[dairyisscary]

glad to see this is getting some attention, thanks @jerelmiller.

if you get a fix out, even if its in an alpha build or something, i'll happily test it for you in a substantially sized application and report any problems I find (our multi hundred thousand line app basically exclusively uses this fetch policy combo + "no-cache" in a few instances).

[jerelmiller]

@dairyisscary sounds great! I've stepped away from this a bit as I've been focused on adding suspense support to the client, but I'm hopeful I can come back to this sometime soon to come up with a better solution than whats in this PR.

[jerelmiller]

It appears this was fixed by #10631 and is no longer needed. I'm going to go ahead and close this out as this issue has already been solved.