Referential identity of arrays lost when setting the cache in client resolvers

[NickClark]

Intended outcome:
When I change cache entries, using either read/writeQuery or read/writeFragment in a client resolver, it should only change the property intended, and leave the referential integrity of everything else in tact.

Actual outcome:
Currently, it causes all properties to lose referential integrity. The arrays are changed. Although it seems when loading data from normal server requests behaves properly

How to reproduce the issue:
Just a rough example I was playing with. I can try to continue cleaning it up if necessary.
Make sure and start the server first. Clicking the refetch button will result in every other fetch having the nested here property matching (correct and intended). Clicking the setActive button will result in the here prop never being referentially sound (broken, unintended)

Server: https://codesandbox.io/s/6z2310j19z
Client: https://codesandbox.io/s/y0yyvp8pzj

EDIT: Ok... so I was able to simplify it. Originals left for reference if desired
https://codesandbox.io/s/oqnk90k68q

Versions

  System:
    OS: macOS High Sierra 10.13.6
  Binaries:
    Node: 10.15.0
    Yarn: 1.15.2
    npm: 6.4.1
  npmPackages:
    apollo-angular: ^1.5.0 => 1.5.0 
    apollo-cache-inmemory: ~1.6.0-beta.1 => 1.6.0-beta.1 
    apollo-client: ^2.6.0-beta.1 => 2.6.0-beta.1 
    apollo-link: ^1.2.11 => 1.2.11

[NickClark]

Added a new, better example to the original description. It seems like this hast to do with arrays only. Seems like a serious bug for people like me who depend on simple identity checks in our view code. The objects inside the array are still good, but the array itself changes every time.

[benjamn]

I believe we could fix this by using a similar kind of caching for executeSubSelectedArray as we use for executeSelectionSet, but the cost of caching executeSubSelectedArray is higher, because one of the inputs is an arbitrarily long array.

In my original benchmarking in #3394, I concluded that caching executeSubSelectedArray was not worth the overhead, especially since array identities are preserved when the parent object is unchanged. As a side note, that's also why we don't cache executeField—there are just too many simple scalar fields that are cheap to (re)compute without caching, so the benefit of caching their values does not outweigh the (small) additional overhead.

In short, if we proceed with your recommendation here, we will need your help to validate the hypothesis that providing === equality for unchanged arrays actually improves overall application performance, given the additional cost of caching.

[NickClark]

I haven't taken a deep look into the issues you've described, Right now it's just troublesome in the view to have to check every element of an array just to decipher if it has changed. The logic is beautifully simple when normal immutability rules are followed. But there may also be a deeper issue here as well. Since in my current project, simply checking the identity of the elements was not enough, since there was, more deeply nested, arrays in the state. Oddly enough, nothing seemed to have changed, but nothing nested in the array had maintained identity. I have been really enjoying/getting used to the idea of client state in Apollo, but little issues like these seem to crop up, making me wonder if not many people use Apollo for local state, yet. Anyways, I'll help however I can. Right now this one issue is problematic and really inconsistent from how just the normal loading of new data from the network works (which "just works").

[benjamn]

Ok, this appears to be fixed if you update to the latest beta version of apollo-cache-inmemory (1.6.0-beta.4): https://codesandbox.io/s/pjz6wyzp30

Following up on my previous concerns, it turns out we can compute the cache key using the object identity of the array that was read from the cache (in other words, the array that gets passed into executeSubSelectedArray), rather than traversing the whole array. We're still missing out on the opportunity to reuse the previous array when all its elements are shallow-identical but the array itself is a different object, but that seems pretty uncommon, so I'm not too worried about creating a new array object in that edge case.

Relevant commits: f3091d6, 5693aa0

[NickClark]

Wow! Nice! I'll check it out hopefully today. Thanks so much for getting on this so quickly!

[NickClark]

Seems to be working well. Thanks!

[benjamn]

Closing since we just published the final version(s) of [email protected] (and [email protected]) to npm, including the fix for this issue! See CHANGELOG.md if you're curious about the other changes in this release.