Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various improvements around previousResult/newData change detection. #4032

Merged
merged 6 commits into from
Oct 22, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Avoid fallible previousResult === newData.result check.
Should help fix #3992.

As #3992 (and specifically the reproduction that @OurMajesty provided at
https://codesandbox.io/s/q7rkm6y1ow) demonstrates, since the InMemoryCache
may return the same object for multiple reads (when the data have not
changed), the previousResult is often exactly (===) the same object as
newData.result, which was causing the code removed by this commit to
return early instead of broadcasting the watch.

This early return was unsafe, because the contents of the object may have
changed since they were previously broadcast, so we actually do need to
report those modifications, even though the object reference is the same.

In other words, `previousResult === newData.result` does not imply
"nothing has changed," as I mistakenly assumed in this discussion:
#3394 (comment)

In the longer term, I would like to eliminate the previousResult logic
entirely, since it seems redundant now that we can precisely track changes
to the store, but for now it's important for backwards compatibility.
Specifically, previousResult still provides a way to prefer the previous
object if it is structurally identical to the new object, though that
preference is moot when `previousResult === newData.result`.

The previousResult object should never be used as a basis for skipping
broadcasts. Only the application developer can decide whether === object
identity means it's safe to skip rendering, likely in the context of a
larger design which treats cache results as immutable data. The job of the
InMemoryCache is to make no unsound assumptions, and broadcast results
whenever they may have changed.
  • Loading branch information
benjamn committed Oct 20, 2018
commit d6a673fbc1444e115e90cc9e4c7fa3fc67bb7e56
15 changes: 3 additions & 12 deletions packages/apollo-cache-inmemory/src/inMemoryCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,20 +339,11 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
// This method is wrapped in the constructor so that it will be called only
// if the data that would be broadcast has changed.
private maybeBroadcastWatch(c: Cache.WatchOptions) {
const previousResult = c.previousResult && c.previousResult();

const newData = this.diff({
c.callback(this.diff({
query: c.query,
variables: c.variables,
previousResult,
previousResult: c.previousResult && c.previousResult(),
optimistic: c.optimistic,
});

if (previousResult &&
previousResult === newData.result) {
return;
}

c.callback(newData);
}));
}
}