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

Don't fire any query observables if the query result didn't change #402

Merged
merged 7 commits into from
Jul 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ Expect active development and potentially significant breaking changes in the `0

### vNEXT
- Added the `batchInterval` option to ApolloClient that allows you to specify the width of the batching interval as per your app's needs. [Issue #394](https://github.com/apollostack/apollo-client/issues/394) and [PR #395](https://github.com/apollostack/apollo-client/pull/395).

- Stringify `storeObj` for error message in `diffFieldAgainstStore`.
- Fix map function returning `undefined` in `removeRefsFromStoreObj`. [PR #393](https://github.com/apollostack/apollo-client/pull/393)
- Added deep result comparison so that observers are only fired when the data associated with a particular query changes. This change eliminates unnecessary re-renders and improves UI performance. [PR #402](https://github.com/apollostack/apollo-client/pull/402) and [Issue #400](https://github.com/apollostack/apollo-client/issues/400).

- Added a "noFetch" option to WatchQueryOptions that only returns available data from the local store (even it is incomplete). The `ObservableQuery` returned from calling `watchQuery` now has `options`, `queryManager`, and `queryId`. The `queryId` can be used to read directly from the state of `apollo.queries`. [Issue #225](https://github.com/apollostack/apollo-client/issues/225), [Issue #342](https://github.com/apollostack/apollo-client/issues/342), and [PR #385](https://github.com/apollostack/apollo-client/pull/385).

Expand Down
39 changes: 27 additions & 12 deletions src/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
queryManager.startQuery(
queryId,
options,
queryManager.queryListenerForObserver(options, observer)
queryManager.queryListenerForObserver(queryId, options, observer)
);
return retQuerySubscription;
};
Expand Down Expand Up @@ -193,14 +193,16 @@ export class QueryManager {
private queryTransformer: QueryTransformer;
private queryListeners: { [queryId: string]: QueryListener };

// A map going from queryId to the last result/state that the queryListener was told about.
private queryResults: { [queryId: string]: ApolloQueryResult };

private idCounter = 0;

private scheduler: QueryScheduler;
private batcher: QueryBatcher;
private batchInterval: number;

// A map going from an index (i.e. just like an array index, except that we can remove
// some of them) to a promise that has not yet been resolved. We use this to keep
// A map going from a requestId to a promise that has not yet been resolved. We use this to keep
// track of queries that are inflight and reject them in case some
// destabalizing action occurs (e.g. reset of the Apollo store).
private fetchQueryPromises: { [requestId: string]: {
Expand Down Expand Up @@ -241,6 +243,7 @@ export class QueryManager {
this.pollingTimers = {};
this.batchInterval = batchInterval;
this.queryListeners = {};
this.queryResults = {};

this.scheduler = new QueryScheduler({
queryManager: this,
Expand Down Expand Up @@ -351,6 +354,7 @@ export class QueryManager {
// Returns a query listener that will update the given observer based on the
// results (or lack thereof) for a particular query.
public queryListenerForObserver(
queryId: string,
options: WatchQueryOptions,
observer: Observer<ApolloQueryResult>
): QueryListener {
Expand Down Expand Up @@ -378,17 +382,22 @@ export class QueryManager {
console.error('Unhandled error', apolloError, apolloError.stack);
}
} else {
const resultFromStore = readSelectionSetFromStore({
store: this.getDataWithOptimisticResults(),
rootId: queryStoreValue.query.id,
selectionSet: queryStoreValue.query.selectionSet,
variables: queryStoreValue.variables,
returnPartialData: options.returnPartialData || options.noFetch,
fragmentMap: queryStoreValue.fragmentMap,
});
const resultFromStore = {
data: readSelectionSetFromStore({
store: this.getDataWithOptimisticResults(),
rootId: queryStoreValue.query.id,
selectionSet: queryStoreValue.query.selectionSet,
variables: queryStoreValue.variables,
returnPartialData: options.returnPartialData || options.noFetch,
fragmentMap: queryStoreValue.fragmentMap,
}),
};

if (observer.next) {
observer.next({ data: resultFromStore });
if (this.isDifferentResult(queryId, resultFromStore )) {
this.queryResults[queryId] = resultFromStore;
observer.next(resultFromStore);
}
}
}
}
Expand Down Expand Up @@ -766,6 +775,12 @@ export class QueryManager {
});
}

// Given a query id and a new result, this checks if the old result is
// the same as the last result for that particular query id.
private isDifferentResult(queryId: string, result: ApolloQueryResult): boolean {
return !isEqual(this.queryResults[queryId], result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work in all cases right? Don't you need to use deepEqual?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nvm, this is deep

}

private broadcastQueries() {
const queries = this.getApolloState().queries;
forOwn(this.queryListeners, (listener: QueryListener, queryId: string) => {
Expand Down
58 changes: 51 additions & 7 deletions test/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ describe('QueryManager', () => {

const data3 = {
people_one: {
name: 'Luke Skywalker has a new name',
name: 'Luke Skywalker has a new name and age',
},
};

Expand All @@ -683,7 +683,7 @@ describe('QueryManager', () => {
},
{
request: { query: query, variables },
result: { data: data2 },
result: { data: data3 },
}
);

Expand Down Expand Up @@ -1467,9 +1467,7 @@ describe('QueryManager', () => {
});

function checkDone() {
// If we make sure queries aren't called twice if the result didn't change, handle2Count
// should change to 1
if (handle1Count === 1 && handle2Count === 2) {
if (handle1Count === 1 && handle2Count === 1) {
done();
}

Expand Down Expand Up @@ -1546,7 +1544,7 @@ describe('QueryManager', () => {
queryManager.query({
query: query2,
});
} else if (handle1Count === 3 &&
} else if (handle1Count === 2 &&
result.data['people_one'].name === 'Luke Skywalker has a new name') {
// 3 because the query init action for the second query causes a callback
assert.deepEqual(result.data, {
Expand Down Expand Up @@ -1736,7 +1734,7 @@ describe('QueryManager', () => {
});

setTimeout(() => {
assert.equal(handleCount, 4);
assert.equal(handleCount, 3);
done();
}, 400);
});
Expand Down Expand Up @@ -2732,6 +2730,52 @@ describe('QueryManager', () => {
done();
}, 100);
});

it('should not fire next on an observer if there is no change in the result', (done) => {
const query = gql`
query {
author {
firstName
lastName
}
}`;

const data = {
author: {
firstName: 'John',
lastName: 'Smith',
},
};
const networkInterface = mockNetworkInterface(
{
request: { query },
result: { data },
},

{
request: { query },
result: { data },
}
);
const queryManager = new QueryManager({
store: createApolloStore(),
reduxRootKey: 'apollo',
networkInterface,
});
const handle = queryManager.watchQuery({ query });
let timesFired = 0;
handle.subscribe({
next(result) {
timesFired += 1;
assert.deepEqual(result, { data });
},
});
queryManager.query({ query }).then((result) => {
assert.deepEqual(result, { data });
assert.equal(timesFired, 1);
done();
});
});
});

function testDiffing(
Expand Down