Skip to content

Commit

Permalink
Stop reading from cache in ObservableQuery#getCurrentResult.
Browse files Browse the repository at this point in the history 
Since getCurrentResult must return its result synchronously, reading from
the cache in this method prevents us from making cache reads asynchronous
in the future.

This may be a breaking change for code that attempts to read cache data
from an ObservableQuery immediately after calling watchQuery, but that's a
behavior that should never have been enforced by our test suite in the
first place.
  • Loading branch information
@benjamn
benjamn committed Nov 11, 2019
1 parent 8a0d73f commit 01376a3
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 105 deletions.
73 changes: 26 additions & 47 deletions src/core/ObservableQuery.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,46 +137,44 @@ 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
* @return {data: Object, error: ApolloError, loading: boolean, networkStatus: number, partial: boolean}
*/
public getCurrentResult(): ApolloCurrentQueryResult<TData> {
if (this.isTornDown) {
const { lastResult } = this;
return {
data: !this.lastError && lastResult && lastResult.data || void 0,
error: this.lastError,
loading: false,
networkStatus: NetworkStatus.error,
};
}

const { data, partial } = this.queryManager.getCurrentQueryResult(this);
const queryStoreValue = this.queryManager.queryStore.get(this.queryId);
let result: ApolloQueryResult<TData>;

const { lastResult, lastError } = this;
const { fetchPolicy } = this.options;

const isNetworkFetchPolicy =
fetchPolicy === 'network-only' ||
fetchPolicy === 'no-cache';

const networkStatus =
lastError ? NetworkStatus.error :
lastResult ? lastResult.networkStatus :
isNetworkFetchPolicy ? NetworkStatus.loading :
NetworkStatus.ready;

const result: ApolloCurrentQueryResult<TData> = {
data: !lastError && lastResult && lastResult.data || void 0,
error: this.lastError,
loading: isNetworkRequestInFlight(networkStatus),
networkStatus,
partial: false,
};

if (this.isTornDown) {
return result;
}

const queryStoreValue = this.queryManager.queryStore.get(this.queryId);
if (queryStoreValue) {
const { networkStatus } = queryStoreValue;

if (hasError(queryStoreValue, this.options.errorPolicy)) {
return {
return Object.assign(result, {
data: void 0,
loading: false,
networkStatus,
error: new ApolloError({
graphQLErrors: queryStoreValue.graphQLErrors,
networkError: queryStoreValue.networkError,
}),
};
});
}

// Variables might have been added dynamically at query time, when
Expand All @@ -192,38 +190,19 @@ export class ObservableQuery<
this.variables = this.options.variables;
}

result = {
data,
Object.assign(result, {
loading: isNetworkRequestInFlight(networkStatus),
networkStatus,
} as ApolloQueryResult<TData>;
});

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/apollostack/apollo-client/issues/707
// Basically: is there a query in flight right now (modolo the next tick)?
const loading = isNetworkFetchPolicy ||
(partial && fetchPolicy !== 'cache-only');

result = {
data,
loading,
networkStatus: loading ? NetworkStatus.loading : NetworkStatus.ready,
} as ApolloQueryResult<TData>;
}

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

return { ...result, partial };
return result;
}

// Compares newResult to the snapshot we took of this.lastResult when it was
Expand Down
2 changes: 1 addition & 1 deletion src/core/QueryManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1047,7 +1047,7 @@ export class QueryManager<TStore> {
this.queries.delete(queryId);
}

public getCurrentQueryResult<T>(
private getCurrentQueryResult<T>(
observableQuery: ObservableQuery<T>,
optimistic: boolean = true,
): {
Expand Down
83 changes: 38 additions & 45 deletions src/core/__tests__/ObservableQuery.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,11 +306,8 @@ describe('ObservableQuery', () => {
} else if (handleCount === 3) {
expect(stripSymbols(result.data)).toEqual(data2);
// go back to first set of variables
await observable.setOptions({ variables });
const current = observable.getCurrentResult();
const current = await observable.setOptions({ variables });
expect(stripSymbols(current.data)).toEqual(data);
const secondCurrent = observable.getCurrentResult();
expect(current.data).toEqual(secondCurrent.data);
resolve();
}
});
Expand Down Expand Up @@ -1461,7 +1458,7 @@ describe('ObservableQuery', () => {
loading: true,
data: undefined,
networkStatus: 1,
partial: true,
partial: false,
});

setTimeout(
Expand All @@ -1470,39 +1467,13 @@ describe('ObservableQuery', () => {
loading: true,
data: undefined,
networkStatus: 1,
partial: true,
partial: false,
});
}),
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,
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 @@ -1592,7 +1563,7 @@ describe('ObservableQuery', () => {
}).then(resolve, reject);
});

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

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

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

if (handleCount === 1) {
expect(subResult).toEqual({
data: dataOne,
data: void 0,
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 @@ -1698,20 +1678,30 @@ describe('ObservableQuery', () => {
loading,
networkStatus,
} = observable.getCurrentResult();

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

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

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

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

expect(stripSymbols(observable.getCurrentResult())).toEqual({
data: undefined,
loading: true,
Expand All @@ -1750,14 +1741,16 @@ describe('ObservableQuery', () => {
loading,
networkStatus,
} = observable.getCurrentResult();
expect(subResult).toEqual({
data,
loading,
networkStatus,
stale: false,
});

if (handleCount === 2) {
if (handleCount === 1) {
expect(subResult).toEqual({
data,
loading,
networkStatus,
partial: false,
stale: false,
});
} else if (handleCount === 2) {
expect(stripSymbols(subResult)).toEqual({
data: dataTwo,
loading: false,
Expand Down
21 changes: 9 additions & 12 deletions src/react/hooks/__tests__/useQuery.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,42 +382,39 @@ describe('useQuery Hook', () => {
cache: new InMemoryCache()
});

const onErrorMock = jest.fn();
let onError;
const onErrorPromise = new Promise(resolve => onError = resolve);

let renderCount = 0;
const Component = () => {
const { loading, error, refetch, data, networkStatus } = useQuery(
query,
{
onError: onErrorMock,
onError,
notifyOnNetworkStatusChange: true
}
);

switch (renderCount) {
case 0:
switch (++renderCount) {
case 1:
expect(loading).toBeTruthy();
break;
case 1:
case 2:
expect(loading).toBeFalsy();
expect(error).toBeDefined();
expect(error!.message).toEqual('Network error: Oh no!');
setTimeout(() => {
expect(onErrorMock.mock.calls.length).toBe(1);
refetch();
});
onErrorPromise.then(() => refetch());
break;
case 2:
case 3:
expect(loading).toBeTruthy();
break;
case 3:
case 4:
expect(loading).toBeFalsy();
expect(data).toEqual(resultData);
break;
default: // Do nothing
}

renderCount += 1;
return null;
};

Expand Down

0 comments on commit 01376a3

Please sign in to comment.