Skip to content

Commit

Permalink
Read from cache again in ObservableQuery#getCurrentResult.
Browse files Browse the repository at this point in the history
This partially reverts two commits from PR #5565, preserving related
improvements that have happened in the meantime:

* Revert "Remove partial field from ApolloCurrentQueryResult type."
  This reverts commit ca2cef6.

* Revert "Stop reading from cache in ObservableQuery#getCurrentResult."
  This reverts commit 01376a3.

One of the motivations behind the original changes was to lay the
foundations for eventually supporting asynchronous (Promise-based) cache
reads, but that form of asynchrony is no longer a goal of Apollo Client
3.0 or any planned future version, so there's not as much benefit to
weakening getCurrentResult in this way.

On the other hand, allowing getCurrentResult to read synchronously from
the cache ensures that custom field read functions can return new results
immediately, which helps with issues like #5782.

Immediate cache reads have also been the behavior of Apollo Client for
much longer than #5565, which makes this change appealing from an
ease-of-upgrading perspective.
  • Loading branch information
benjamn committed Jan 14, 2020
1 parent 617b1db commit ecfb48f
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 35 deletions.
22 changes: 16 additions & 6 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { isNonEmptyArray } from '../utilities/common/arrays';

export type ApolloCurrentQueryResult<T> = ApolloQueryResult<T> & {
error?: ApolloError;
partial?: boolean;
};

export interface FetchMoreOptions<
Expand Down Expand Up @@ -131,8 +132,12 @@ export class ObservableQuery<
}

public getCurrentResult(): ApolloCurrentQueryResult<TData> {
const { lastResult, lastError } = this;
const { fetchPolicy } = this.options;
const {
lastResult,
lastError,
options: { fetchPolicy },
} = this;

const isNetworkFetchPolicy =
fetchPolicy === 'network-only' ||
fetchPolicy === 'no-cache';
Expand All @@ -145,7 +150,7 @@ export class ObservableQuery<

const result: ApolloCurrentQueryResult<TData> = {
data: !lastError && lastResult && lastResult.data || void 0,
error: this.lastError,
error: lastError,
loading: isNetworkRequestInFlight(networkStatus),
networkStatus,
stale: lastResult ? lastResult.stale : false,
Expand All @@ -155,6 +160,9 @@ export class ObservableQuery<
return result;
}

const { data, partial } = this.queryManager.getCurrentQueryResult(this);
Object.assign(result, { data, partial });

const queryStoreValue = this.queryManager.queryStore.get(this.queryId);
if (queryStoreValue) {
const { networkStatus } = queryStoreValue;
Expand All @@ -176,11 +184,10 @@ export class ObservableQuery<
// the original `ObservableQuery`. We'll update the observable query
// variables here to match, so retrieving from the cache doesn't fail.
if (queryStoreValue.variables) {
this.options.variables = {
this.variables = this.options.variables = {
...this.options.variables,
...(queryStoreValue.variables as TVariables),
};
this.variables = this.options.variables;
}

Object.assign(result, {
Expand All @@ -193,7 +200,10 @@ export class ObservableQuery<
}
}

this.updateLastResult(result);
if (!partial) {
result.stale = false;
this.updateLastResult(result);
}

return result;
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,7 @@ export class QueryManager<TStore> {
this.queries.delete(queryId);
}

private getCurrentQueryResult<T>(
public getCurrentQueryResult<T>(
observableQuery: ObservableQuery<T>,
optimistic: boolean = true,
): {
Expand Down
70 changes: 43 additions & 27 deletions src/core/__tests__/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,7 @@ describe('ObservableQuery', () => {
loading: false,
networkStatus: 7,
stale: false,
partial: false,
});
resolve();
});
Expand All @@ -1459,6 +1460,7 @@ describe('ObservableQuery', () => {
data: undefined,
networkStatus: 1,
stale: false,
partial: true,
});

setTimeout(
Expand All @@ -1468,12 +1470,40 @@ describe('ObservableQuery', () => {
data: undefined,
networkStatus: 1,
stale: false,
partial: true,
});
}),
0,
);
});

itAsync('returns results from the store immediately', (resolve, reject) => {
const queryManager = mockQueryManager(reject, {
request: { query, variables },
result: { data: dataOne },
});

return queryManager.query({ query, variables }).then((result: any) => {
expect(stripSymbols(result)).toEqual({
data: dataOne,
loading: false,
networkStatus: 7,
stale: false,
});
const observable = queryManager.watchQuery({
query,
variables,
});
expect(stripSymbols(observable.getCurrentResult())).toEqual({
data: dataOne,
loading: true,
networkStatus: NetworkStatus.loading,
stale: false,
partial: false,
});
}).then(resolve, reject);
});

itAsync('returns errors from the store immediately', (resolve, reject) => {
const queryManager = mockQueryManager(reject, {
request: { query, variables },
Expand Down Expand Up @@ -1563,7 +1593,7 @@ describe('ObservableQuery', () => {
}).then(resolve, reject);
});

itAsync('returns partial data from the store', (resolve, reject) => {
itAsync('returns partial data from the store immediately', (resolve, reject) => {
const superQuery = gql`
query superQuery($id: ID!) {
people_one(id: $id) {
Expand Down Expand Up @@ -1600,10 +1630,11 @@ describe('ObservableQuery', () => {
});

expect(observable.getCurrentResult()).toEqual({
data: void 0,
data: dataOne,
loading: true,
networkStatus: 1,
stale: false,
partial: true,
});

// we can use this to trigger the query
Expand All @@ -1618,28 +1649,19 @@ describe('ObservableQuery', () => {

if (handleCount === 1) {
expect(subResult).toEqual({
data: void 0,
data: dataOne,
loading: true,
networkStatus: 1,
stale: false,
});

} else if (handleCount === 2) {
expect(subResult).toEqual({
data: dataOne,
loading: false,
networkStatus: 7,
stale: false,
});

} else if (handleCount === 3) {
expect(subResult).toEqual({
data: superDataOne,
loading: false,
networkStatus: 7,
stale: false,
});

resolve();
}
});
Expand Down Expand Up @@ -1670,6 +1692,7 @@ describe('ObservableQuery', () => {
loading: true,
networkStatus: 1,
stale: false,
partial: false,
});

subscribeAndCount(reject, observable, (handleCount, subResult) => {
Expand All @@ -1679,29 +1702,21 @@ describe('ObservableQuery', () => {
networkStatus,
} = observable.getCurrentResult();

expect(subResult).toEqual({
data,
loading,
networkStatus,
stale: false,
});

if (handleCount === 1) {
expect(stripSymbols(subResult)).toEqual({
data: void 0,
loading: true,
networkStatus: NetworkStatus.loading,
expect(subResult).toEqual({
data,
loading,
networkStatus,
stale: false,
partial: false,
});

} else if (handleCount === 2) {
expect(stripSymbols(subResult)).toEqual({
data: dataTwo,
loading: false,
networkStatus: NetworkStatus.ready,
networkStatus: 7,
stale: false,
});

resolve();
}
});
Expand All @@ -1727,12 +1742,12 @@ describe('ObservableQuery', () => {
variables,
fetchPolicy: 'no-cache',
});

expect(stripSymbols(observable.getCurrentResult())).toEqual({
data: undefined,
loading: true,
networkStatus: 1,
stale: false,
partial: false,
});

subscribeAndCount(reject, observable, (handleCount, subResult) => {
Expand All @@ -1748,6 +1763,7 @@ describe('ObservableQuery', () => {
loading,
networkStatus,
stale: false,
partial: false,
});
} else if (handleCount === 2) {
expect(stripSymbols(subResult)).toEqual({
Expand Down
3 changes: 2 additions & 1 deletion src/react/data/QueryData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ export class QueryData<TData, TVariables> extends OperationData {
} else {
// Fetch the current result (if any) from the store.
const currentResult = this.currentObservable.query!.getCurrentResult();
const { loading, networkStatus, errors } = currentResult;
const { loading, partial, networkStatus, errors } = currentResult;
let { error, data } = currentResult;

// Until a set naming convention for networkError and graphQLErrors is
Expand Down Expand Up @@ -390,6 +390,7 @@ export class QueryData<TData, TVariables> extends OperationData {
const { partialRefetch } = options;
if (
partialRefetch &&
partial &&
(!data || Object.keys(data).length === 0) &&
fetchPolicy !== 'cache-only'
) {
Expand Down

0 comments on commit ecfb48f

Please sign in to comment.