Preserve referential equality from previous result in the store #1136

calebmer · 2017-01-05T22:42:49Z

This PR implements the preservation of referential equality across reads from the store for data that has not changed. So say you have an array of [a, b, c] and a mutation adds a new item, d, to make [a, b, c, d]. Then the following should hold last[0] === next[0] && last[1] === next[1] && last[2] === next[2] while the array itself is not necessarily referentially equal (last !== next). This makes it easier to create optimizations in the UI layer to only re-render data which changed. Specifically this would make the implementation of shouldComponentUpdate in React apps much cleaner.

This PR is not yet finished, but I’d like an initial review of what’s there. I’m going to work on some other issues for a bit then come back to this. The implementation is a little messy as the graphql-anywhere resolver and resultMapper need to work together to both identify which values have previous results and if those previous results equal the new result. The implementation gets a little cleaner if we remove support for arrays whose order changes (e.g. [a, b, c] -> [b, c, a]), but that means we wouldn’t support the case where an item is added to the beginning of an array which is pretty important (e.g. [a, b, c] -> [d, a, b, c]).

What’s left to do is wiring this up with wherever it is called so that previousResult values actually get passed down into diffQueryAgainstStore.

Closes #695

If this PR is a new feature, 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
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change

stubailo

Minor comments

stubailo · 2017-01-06T00:27:10Z

src/data/readFromStore.ts

-    // if this is an object scalar, it must be a json blob and we have to unescape it
+    // If the JSON blob is the same now as in the previous result, return the previous result to
+    // maintain referential equality.
+    if (idValue.previousResult && isEqual(idValue.previousResult[fieldName], fieldValue.json)) {


I think we can use === here instead of isEqual

Test fails when I do that. It’s because two requests will never return the exact same object (when compared with ===), so we have to do a deep equals.

OK I'm fine with this for now - let's do some benchmarks later and see if it is ever a problem!

I'm not sure I follow here. Surely once written to the store the object doesn't change, right? It's only when another request fetches the same object that the reference changes. If that's the case, then we could first check referential equality and only if that fails apply isEqual.

@helfer isEqual always does a referential check first. If you’d like we could do another one here to make the intention explicit.

If you don’t care about referential equality when a new JSON object comes back from the API (which when a deep comparison is performed is actually the same object) we could just remove this entirely as the edge case may not be worth supporting at all. I only really added it to get this test to pass, otherwise it’s a safe removal.

stubailo · 2017-01-06T00:29:10Z

src/data/readFromStore.ts

+    //
+    // While we do a shallow comparison of objects, we do a deep comparison of arrays.
+    const sameAsPreviousResult = Object.keys(resultFields).reduce(
+      (same, key) => {


Code style - I'd put these args on the previous line.

Yeah, this line went through multiple iterations.

stubailo · 2017-01-06T00:30:02Z

src/data/readFromStore.ts

+        // Flatten out the field values before comparing them. Non-arrays will turn into singleton
+        // arrays and multi-dimensional arrays will be flattened out. Depth doesn’t matter in this
+        // case, we just need to check that all items are equal.
+        const next = flattenArray(resultFields[key]);


This would definitely pass through a false positive if the arrays had the same items in flattened form but had different shapes.

You’re right. I initially patched isEqual to do shallow object checks when a config flag was passed, but that felt wrong so I changed it to this. There is probably a better solution.

helfer

This is really awesome! 👍 I'm especially liking the thorough yet relatively succinct and clean tests.

I agree that it may feel a bit contrived, but it fits the graphql-anywhere pattern and I don't see how we could improve much on it while still using graphql-anywhere. I believe it will take a moment to understand what's going on for folks who aren't familiar with graphql-anywhere (eg. practically everyone outside the apollo team) so I do think it would help to add a comment at the top that explains how the resolver and resultMapper play together to make things work.

Actually, that reminds me that we should still write a blog post at some time that explains what graphql-anywhere is and does!

helfer · 2017-01-06T16:35:59Z

src/data/readFromStore.ts

+      // Flatten out the field values before comparing them. Non-arrays will turn into singleton
+      // arrays and multi-dimensional arrays will be flattened out. Depth doesn’t matter in this
+      // case, we just need to check that all items are equal.
+      const next = flattenArray(resultFields[key]);


What are the performance implications of this? I'm not too worried about deeply nested arrays at this point in time, but what about one-dimensional arrays? If we're copying every array every time to make a comparison that could amount to a fair bit of copying.

Although... I once said never optimize something you don't already know needs optimizing. So that might count here as well.

My guess would be it isn’t a huge deal, but I’m going to change it anyway. @stubailo pointed out it could be a source of bugs by creating false positives.

helfer · 2017-01-06T16:39:05Z

src/data/readFromStore.ts

-    // if this is an object scalar, it must be a json blob and we have to unescape it
+    // If the JSON blob is the same now as in the previous result, return the previous result to
+    // maintain referential equality.
+    if (idValue.previousResult && isEqual(idValue.previousResult[fieldName], fieldValue.json)) {


I'm not sure I follow here. Surely once written to the store the object doesn't change, right? It's only when another request fetches the same object that the reference changes. If that's the case, then we could first check referential equality and only if that fails apply isEqual.

helfer · 2017-01-06T16:42:24Z

test/diffAgainstStore.ts

+        previousResult,
+      });
+
+      assert.deepEqual(result, queryResult);


I guess this assertion wouldn't strictly be necessary, but you put it in to speed up debugging if things are still deepEqual but not strictly equal?

Yeah, it’s more of a quick sanity check then anything else. For instance result might not deep equal queryResult if we wrote result.a.b = 10 on line 432 but would still strict equal previousResult.

helfer · 2017-01-06T16:45:17Z

test/diffAgainstStore.ts

@@ -346,4 +347,347 @@ describe('diffing queries against the store', () => {
      });
    });
  });
+


These tests look really neat! For some reason that makes me really happy.

stubailo · 2017-01-06T17:14:57Z

@helfer when I talked to Caleb he mentioned that isEqual checks referential equality first anyway for performance.

calebmer · 2017-01-06T18:45:10Z

Added some commits to address comments and improve file legibility. I’m going to work on getting the previousResult passed into the appropriate call sites for diffQueryAgainstStore now. I’ll ping the thread again once that’s done 👍

…uality

calebmer · 2017-01-06T22:13:22Z

Ok, so I added the previousResult option here which I think is all that really matters? Because it appears that is the code path which creates results for watchQuery which is what React uses. Whenever the store changes this code path generates a new result for the watchQuery observers.

Other locations which we could add a previousResult to is as follows. Let me know if a previousResult should be added to any of these locations:

stubailo · 2017-01-06T22:46:11Z

CHANGELOG.md

@@ -15,6 +15,7 @@ Expect active development and potentially significant breaking changes in the `0
 - Remove lodash as a production dependency [PR #1122](https://github.com/apollostack/apollo-client/pull/1122)
 - Minor fix to write to `ROOT_SUBSCRIPTION` ID in the store for subscription results. [PR #1122](https://github.com/apollostack/apollo-client/pull/1127)
 - Remove `whatwg-fetch` polyfill dependency and instead warn when a global `fetch` implementation is not found. [PR #1134](https://github.com/apollostack/apollo-client/pull/1134)
+- New results from `watchQuery` are referentially equal (so `a === b`) to previous results if nothing changed in the store for a better UI integration experience when determining what changed. [PR #1136](https://github.com/apollostack/apollo-client/pull/1136)


This misses the important point that this applies to sub-objects in the result, not just the whole result (which was true before!)

stubailo · 2017-01-06T22:58:39Z

We should write a blog post about this when it ships. I think it will do a lot to demonstrate the attention to detail of what we're doing.

helfer · 2017-01-09T14:44:56Z

src/data/readFromStore.ts

+  }
+  // Otherwise let us compare all of the array items (which are potentially nested arrays!) to see
+  // if they are equal.
+  return a.reduce((same, item, i) => same && areNestedArrayItemsStrictlyEqual(item, b[i]), true);


With this comparison we'd end up with [1,2] === [1,2,3] because we're only comparing up to the number of items in array a.

Yep, that’s my bad. Comparing length up front is probably good hygiene anyway.

helfer · 2017-01-09T16:01:20Z

Sweet, that's an awesome PR @calebmer!

I added previousResult to getCurrentResult as well because React Apollo uses that in a bunch of places, and I think we need referential equality there as well. It definitely doesn't hurt.

Going to merge this now.

danielgriese · 2017-01-18T07:35:19Z

WUAT? This bugged me for a long time. Awesome! Thank you very much and sorry for the spam... just wanted to show my gratitude! :D

add referential equality with previous store result

a062622

helfer added the in progress label Jan 5, 2017

stubailo reviewed Jan 6, 2017

View reviewed changes

change code style

8738b44

helfer reviewed Jan 6, 2017

View reviewed changes

calebmer added 4 commits January 6, 2017 10:23

refactor nested array comparison in resultMapper

bae06ed

add some high level explanation of what is going on

6468846

move some functions around to help with file legibility

a4931fd

add some more inline comments to JSON equality check

190a86a

calebmer added 7 commits January 6, 2017 11:29

refactor readQueryFromStore to support new options

5833dc4

don’t let addPreviousResultToIdValues mutate values

8476608

use result key instead of fieldName for resolver

e1eae36

compare result keys

08b69dd

use previous result in queryListenerForObserver

249c552

Merge branch 'zero-decimal-six' into refactor/preserve-referential-eq…

a1ae3b6

…uality

code formatting

d314e2f

calebmer added 2 commits January 6, 2017 14:20

add changelog item

ff1af8d

add test for watch query

c1a0b09

stubailo reviewed Jan 6, 2017

View reviewed changes

Update CHANGELOG.md

c97b0f8

helfer reviewed Jan 9, 2017

View reviewed changes

helfer and others added 5 commits January 9, 2017 22:45

add a test for array comparison

d74149f

check array length before comparing items

af36e8e

nitpick code aesthetics

ec2163e

get referential equality in getCurrentResult

f127dbb

add test for currentResult referential equality

24a1a32

helfer merged commit aa06fda into zero-decimal-six Jan 9, 2017

helfer removed the in progress label Jan 9, 2017

calebmer deleted the refactor/preserve-referential-equality branch January 9, 2017 16:06

This was referenced Jan 19, 2017

[Question] Objects within list change after mutation of the list #695

Closed

Performance issues when sharing Redux store #1107

Closed

Poincare mentioned this pull request Jan 30, 2017

Expose a way to turn off referential equality checking #1242

Closed

Crash-- mentioned this pull request Sep 27, 2018

Granular access to query results cozy/cozy-client#193

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve referential equality from previous result in the store #1136

Preserve referential equality from previous result in the store #1136

calebmer commented Jan 5, 2017 •

edited by Urigo

Loading

stubailo left a comment

stubailo Jan 6, 2017

calebmer Jan 6, 2017

stubailo Jan 6, 2017

helfer Jan 6, 2017

calebmer Jan 6, 2017

calebmer Jan 6, 2017

stubailo Jan 6, 2017

calebmer Jan 6, 2017

stubailo Jan 6, 2017

calebmer Jan 6, 2017

helfer left a comment

helfer Jan 6, 2017

calebmer Jan 6, 2017

helfer Jan 6, 2017

helfer Jan 6, 2017

calebmer Jan 6, 2017

helfer Jan 6, 2017

stubailo commented Jan 6, 2017

calebmer commented Jan 6, 2017

calebmer commented Jan 6, 2017

stubailo Jan 6, 2017

calebmer Jan 6, 2017

stubailo commented Jan 6, 2017

helfer Jan 9, 2017

calebmer Jan 9, 2017

helfer commented Jan 9, 2017

danielgriese commented Jan 18, 2017

@@ @@ -346,4 +347,347 @@ describe('diffing queries against the store', () => { @@
                     });
                   });
                 });

Preserve referential equality from previous result in the store #1136

Preserve referential equality from previous result in the store #1136

Conversation

calebmer commented Jan 5, 2017 • edited by Urigo Loading

stubailo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stubailo commented Jan 6, 2017

calebmer commented Jan 6, 2017

calebmer commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stubailo commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer commented Jan 9, 2017

danielgriese commented Jan 18, 2017

calebmer commented Jan 5, 2017 •

edited by Urigo

Loading