Check structural equality in QueryInfo#setDiff, again.

[benjamn]

Revisiting PR #6891, which was reverted by commit c2ef68f.

With the introduction of canonical cache results (#7439), the strict equality check in setDiff is about to become mostly synonymous with deep equality checking (but much faster to check), so we need to deal with the consequences of #6891 one way or another.

As evidence that this change now (after #7439) has no observable impact, notice that we were able to switch back to using !equal(a, b) without needing to fix/update any failing tests, compared to the handful of tests that needed updating in #6891.

However, by switching to deep equality checking, we allow ourselves the option to experiment with disabling (or periodically clearing) the ObjectCanon, which would presumably lead to broadcasting more !== (but deeply equal) results to cache.watch watchers.

With deep equality checking in setDiff, those !== results will get past the setDiff filter in the same cases canonical objects would, so there will be less of a discrepancy in client behavior with/without canonization enabled.

[brainkim]

@benjamn
I’m okay with this if you think it’s a good idea, but I’m concerned about this whole file (QueryInfo.ts) in general. It feels a little bit like we are using equality checks here with InMemoryCache.watch() to implement behavior which would better be encapsulated in a place like QueryManager.fetchQueryByPolicy(), where the fetch policy logic is mostly implemented. The fact of the matter is that setDiff() is only called in one place in this file (in a callback), and in other places QueryInfo.diff is assigned manually, so the main purpose of this method seems to be to notify callback listeners in a setTimeout() callback. Therefore, it feels deeply unclear to me why referential equality, or even structural equality, has semantic meaning here, or what processes it’s supposed to kick off. In my wine-addled opinion, the behavior of this code feels emergent rather than intentional, and I think we should consider refactoring this stuff to clarify why a local cache miss here would trigger a network request somewhere else.

The other concerns are more practical. The studio team indicated that they were having issues with cache.evict not re-triggering network requests (PR 3097 in studio). Is this a use-case we’re supporting? Because I have no idea what cache eviction has to do with queries re-firing. I would like to make sure we have a better handle on their use-case.

Additionally, my current understanding of these optimism caches is that they are performance optimizations, equality checks aside. What is the impact of using structural equality checking here? The unknowns are compounded by the fact that this new canonicalization is layered on top of the optimism-based caching so I am very much in the dark about whether there are good reasons to switch from referential equality to structural equality here.

For these reasons, I am unable to approve or disapprove of this PR. Of course, I’m still somewhat new to this codebase and defer to your expertise.

[benjamn]

Therefore, it feels deeply unclear to me why referential equality, or even structural equality, has semantic meaning here, or what processes it’s supposed to kick off.

@brainkim I agree this pipeline could be simplified, but I think it would be wise to separate the following two questions:

1. How should a watched query react upon receiving new data? This is a deep and open problem space, and it's safe to say our current collection of built-in fetch policies does not come anywhere close to covering all possible use cases / desired behaviors. For example, you could imagine a sophisticated policy that attempts to refetch individual objects with missing fields, to avoid refetching the whole query. The sky is the limit, though simplicity/predictability is at a premium for the common use cases.

2. Should a watched query ever receive the same data twice in a row? Regardless of how the query is supposed to react to new data, it's probably unnecessary and potentially a performance problem to notify the query twice in a row with the "same" data. I put "same" in quotes because sameness could mean either strict referential equality or some notion of deep equality. However, I believe the canonization trick (Efficiently canonicalize InMemoryCache result objects. #7439) forces our hand here, since strict equality checking with canonical objects amounts to deep equality checking. If someone were to disable canonization for performance or debugging reasons, or cleared the ObjectCanon to reclaim memory, it would be unfortunate if this code suddenly started letting more !== results through. If we stick to deep equality checking here, the behavior will be the same whether canonization is happening or not.

My goal with this distinction is to suggest that the second question is much easier to answer (no, watched queries should never receive the same deeply equal data twice in a row), and justifies the changes in this PR, without requiring a full answer to the first question (though I would love to keep thinking about that too).

As for the Studio cache.evict issues, PR #8000 introduces an API we will be encouraging them to use instead of just calling cache.evict to trigger changes, so I'm confident this change won't be a problem for them this time, with guidance from us about how to use cache.refetchQueries. That guidance will be necessary anyway (even without this PR), since the introduction of canonization is likely to trigger the same problems with cache.evict that deep equality checking revealed in #6891, so we have to get to the bottom of those regressions before Studio can use v3.4 anyway.

[brainkim]

I believe the canonization trick (#7439) forces our hand here, since strict equality checking with canonical objects amounts to deep equality checking. If someone were to disable canonization for performance or debugging reasons, or cleared the ObjectCanon to reclaim memory, it would be unfortunate if this code suddenly started letting more !== results through.

Maybe a unit test would be in order, if this is the case? What exactly is the public expectation which would fail if we didn’t use structural equality here?

[benjamn]

@brainkim I'm all for a test, but that will have to wait on making it possible to opt out of canonization (which is something I'm planning for v3.4 but haven't gotten to yet).

[benjamn]

@brainkim Alright, I added a regression test (that I can confirm fails if we use !== to test equality in setDiff) in e3ec93d. Look good to you?

[brainkim]

The pieces are falling into place it seems.