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 dff41e1
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 30 deletions.
37 changes: 34 additions & 3 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 @@ -130,9 +131,18 @@ export class ObservableQuery<
});
}

/**
* Return the result of the query from the local cache as well as some fetching status
* `loading` and `networkStatus` allow to know if a request is in flight
* `partial` lets you know if the result from the local cache is complete or partial
*/
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 @@ -155,6 +165,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 Down Expand Up @@ -191,9 +204,27 @@ export class ObservableQuery<
if (queryStoreValue.graphQLErrors && this.options.errorPolicy === 'all') {
result.errors = queryStoreValue.graphQLErrors;
}

} else {
// We need to be careful about the loading state we show to the user, to try
// and be vaguely in line with what the user would have seen from .subscribe()
// but to still provide useful information synchronously when the query
// will not end up hitting the server.
// See more: https://github.com/apollographql/apollo-client/issues/707
// Basically: is there a query in flight right now?
const loading = isNetworkFetchPolicy ||
(partial && fetchPolicy !== 'cache-only');

Object.assign(result, {
loading,
networkStatus: loading ? NetworkStatus.loading : NetworkStatus.ready,
});
}

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
64 changes: 39 additions & 25 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 @@ -1678,30 +1701,20 @@ describe('ObservableQuery', () => {
loading,
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,
stale: false,
});

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

resolve();
}
});
Expand All @@ -1727,12 +1740,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 +1761,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 dff41e1

Please sign in to comment.