Skip to content

Commit

Permalink
Snapshot observableQuery.lastResult to defend against mutation.
Browse files Browse the repository at this point in the history
Should help with #3992.

Though deep cloning and isEqual testing are obviously more expensive than
relying on === equality, the reality is that lastResult may have been
modified since it was previously recorded, so === equality is useless for
determining isDifferentResult. A defensive copy is the only way to make
sure we get this right.
  • Loading branch information
benjamn committed Oct 20, 2018
1 parent 78e2ad8 commit e66027c
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 11 deletions.
9 changes: 4 additions & 5 deletions packages/apollo-client/src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isEqual, tryFunctionOrLogError } from 'apollo-utilities';
import { isEqual, tryFunctionOrLogError, cloneDeep } from 'apollo-utilities';
import { GraphQLError } from 'graphql';
import { NetworkStatus, isNetworkRequestInFlight } from './networkStatus';
import { Observable, Observer, Subscription } from '../util/Observable';
Expand Down Expand Up @@ -201,7 +201,7 @@ export class ObservableQuery<
}

const result = {
data,
data: cloneDeep(data),
loading: isNetworkRequestInFlight(networkStatus),
networkStatus,
} as ApolloQueryResult<TData>;
Expand All @@ -215,8 +215,7 @@ export class ObservableQuery<
}

if (!partial) {
const stale = false;
this.lastResult = { ...result, stale };
this.lastResult = { ...result, stale: false };
}

return { ...result, partial } as ApolloCurrentResult<TData>;
Expand Down Expand Up @@ -586,7 +585,7 @@ export class ObservableQuery<

const observer: Observer<ApolloQueryResult<TData>> = {
next: (result: ApolloQueryResult<TData>) => {
this.lastResult = result;
this.lastResult = cloneDeep(result);
this.observers.forEach(obs => obs.next && obs.next(result));
},
error: (error: ApolloError) => {
Expand Down
6 changes: 2 additions & 4 deletions packages/apollo-client/src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
getQueryDefinition,
isProduction,
hasDirectives,
isEqual,
} from 'apollo-utilities';

import { QueryScheduler } from '../scheduler/scheduler';
Expand Down Expand Up @@ -609,10 +610,7 @@ export class QueryManager<TStore> {
resultFromStore &&
lastResult.networkStatus === resultFromStore.networkStatus &&
lastResult.stale === resultFromStore.stale &&
// We can do a strict equality check here because we include a `previousResult`
// with `readQueryFromStore`. So if the results are the same they will be
// referentially equal.
lastResult.data === resultFromStore.data
isEqual(lastResult.data, resultFromStore.data)
);

if (isDifferentResult || previouslyHadError) {
Expand Down
3 changes: 1 addition & 2 deletions packages/apollo-client/src/core/__tests__/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,7 @@ describe('ObservableQuery', () => {
const current = observable.currentResult();
expect(stripSymbols(current.data)).toEqual(data);
const secondCurrent = observable.currentResult();
// ensure ref equality
expect(current.data).toBe(secondCurrent.data);
expect(current.data).toEqual(secondCurrent.data);
done();
}
});
Expand Down

0 comments on commit e66027c

Please sign in to comment.