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.

Sign up for GitHub

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

Refactor ObservableQuery#getCurrentResult to reenable immediate delivery of warm cache results. #6710

Merged
merged 16 commits into from
Jul 27, 2020
Merged

Refactor ObservableQuery#getCurrentResult to reenable immediate delivery of warm cache results. #6710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
513d733
Eliminate ApolloCurrentQueryResult in favor of ApolloQueryResult.
benjamn Jul 9, 2020
01a899e
Stop using getCurrentQueryResult in updateQuery.
benjamn Jul 21, 2020
c08043c
Simplify ObservableQuery#getCurrentResult.
benjamn Jul 21, 2020
2192f89
Replace QueryManager#getQueryStoreValue with more specific accessors.
benjamn Jul 22, 2020
383405e
Bring back QueryInfo#getDiff, and privatize updateWatch.
benjamn Jul 22, 2020
022c797
Avoid watching cache for no-cache queries.
benjamn Jul 24, 2020
af2d1e3
Remove unused QueryManager#addQueryListener method.
benjamn Jul 23, 2020
a1cd4f4
Pass QueryInfo into ObservableQuery constructor.
benjamn Jul 22, 2020
06a1dc2
Avoid unnecessarily discarding QueryInfo fields on stop.
benjamn Jul 23, 2020
0cf0cbd
Use this.queryInfo directly, obviating new methods.
benjamn Jul 23, 2020
61e0e62
Use QueryInfo#getDiff in getCurrentQueryResult.
benjamn Jul 22, 2020
7df6ec0
Inline getCurrentQueryResult and simplify its logic.
benjamn Jul 23, 2020
8be3800
Move cache-first fallback logic after first network request.
benjamn Jul 27, 2020
5fe8fee
Avoid loading:true results for warm cache reads when possible.
benjamn Jul 27, 2020
fb3c913
Mention PR #6710 in CHANGELOG.md.
benjamn Jul 27, 2020
fd94a3d
Mention removal of ApolloCurrentQueryResult type in CHANGELOG.md.
benjamn Jul 27, 2020
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
[@igaloly](https://github.com/igaloly) in [#6261](https://github.com/apollographql/apollo-client/pull/6261) and
[@Kujawadl](https://github.com/Kujawadl) in [#6526](https://github.com/apollographql/apollo-client/pull/6526)

- Refactor `ObservableQuery#getCurrentResult` to reenable immediate delivery of warm cache results. As part of this refactoring, the `ApolloCurrentQueryResult` type was eliminated in favor of `ApolloQueryResult`. <br/>
[@benjamn](https://github.com/benjamn) in [#6710](https://github.com/apollographql/apollo-client/pull/6710)

## Improvements

- Errors of the form `Invariant Violation: 42` thrown in production can now be looked up much more easily, by consulting the auto-generated `@apollo/client/invariantErrorCodes.js` file specific to your `@apollo/client` version. <br/>
Expand Down
211 changes: 68 additions & 143 deletions src/core/ObservableQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,9 @@ import {
WatchQueryOptions,
FetchMoreQueryOptions,
SubscribeToMoreOptions,
ErrorPolicy,
} from './watchQueryOptions';
import { QueryStoreValue } from './QueryInfo';
import { Reobserver } from './Reobserver';

export type ApolloCurrentQueryResult<T> = ApolloQueryResult<T> & {
error?: ApolloError;
partial?: boolean;
};
import { QueryInfo } from './QueryInfo';

export interface FetchMoreOptions<
TData = any,
Expand All @@ -45,14 +39,6 @@ export interface UpdateQueryOptions<TVariables> {
variables?: TVariables;
}

export const hasError = (
storeValue: QueryStoreValue,
policy: ErrorPolicy = 'none',
) => storeValue && (
storeValue.networkError ||
(policy === 'none' && isNonEmptyArray(storeValue.graphQLErrors))
);

let warnedAboutUpdateQuery = false;

export class ObservableQuery<
Expand All @@ -77,12 +63,15 @@ export class ObservableQuery<
private lastResult: ApolloQueryResult<TData>;
private lastResultSnapshot: ApolloQueryResult<TData>;
private lastError: ApolloError;
private queryInfo: QueryInfo;

constructor({
queryManager,
queryInfo,
options,
}: {
queryManager: QueryManager<any>;
queryInfo: QueryInfo;
options: WatchQueryOptions<TVariables>;
}) {
super((observer: Observer<ApolloQueryResult<TData>>) =>
Expand All @@ -101,6 +90,8 @@ export class ObservableQuery<

// related classes
this.queryManager = queryManager;

this.queryInfo = queryInfo;
}

public result(): Promise<ApolloQueryResult<TData>> {
Expand Down Expand Up @@ -134,26 +125,11 @@ export class ObservableQuery<
});
}

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

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: lastError,
public getCurrentResult(): ApolloQueryResult<TData> {
const { lastResult, lastError } = this;
const networkStatus = this.queryInfo.networkStatus || NetworkStatus.ready;
const result: ApolloQueryResult<TData> = {
...(lastError ? { error: lastError } : lastResult),
loading: isNetworkRequestInFlight(networkStatus),
networkStatus,
};
Expand All @@ -162,51 +138,40 @@ export class ObservableQuery<
return result;
}

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

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

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

// Variables might have been added dynamically at query time, when
// using `@client @export(as: "varname")` for example. When this happens,
// the variables have been updated in the query store, but not updated on
// 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.options.variables,
...(queryStoreValue.variables as TVariables),
};
}

Object.assign(result, {
loading: isNetworkRequestInFlight(networkStatus),
networkStatus,
});

if (queryStoreValue.graphQLErrors && this.options.errorPolicy === 'all') {
result.errors = queryStoreValue.graphQLErrors;
const { fetchPolicy = 'cache-first' } = this.options;
if (fetchPolicy === 'no-cache' ||
fetchPolicy === 'network-only') {
result.partial = false;
} else if (
!result.data ||
// If this.options.query has @client(always: true) fields, we cannot
// trust result.data, since it was read from the cache without
// running local resolvers (and it's too late to run resolvers now,
// since we must return a result synchronously). TODO In the future
// (after Apollo Client 3.0), we should find a way to trust
// this.lastResult in more cases, and read from the cache only in
// cases when no result has been received yet.
!this.queryManager.transform(this.options.query).hasForcedResolvers
) {
const diff = this.queryInfo.getDiff();
result.partial = !diff.complete;
result.data = (
diff.complete ||
this.options.returnPartialData
) ? diff.result : void 0;
// If the cache diff is complete, and we're using a FetchPolicy that
// terminates after a complete cache read, we can assume the next
// result we receive will have NetworkStatus.ready and !loading.
if (diff.complete &&
result.networkStatus === NetworkStatus.loading &&
(fetchPolicy === 'cache-first' ||
fetchPolicy === 'cache-only')) {
result.networkStatus = NetworkStatus.ready;
result.loading = false;
}
}

if (partial) {
this.resetLastResults();
} else {
this.updateLastResult(result);
}
this.updateLastResult(result);
Comment on lines -205 to +174
Copy link
Member Author

Choose a reason for hiding this comment

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

Always updating this.lastResult makes more sense now that ApolloQueryResult contains the partial boolean field. We last considered this logic in #6417.


return result;
}
Expand Down Expand Up @@ -235,11 +200,7 @@ export class ObservableQuery<
}

public resetQueryStoreErrors() {
const queryStore = this.queryManager.getQueryStoreValue(this.queryId);
if (queryStore) {
queryStore.networkError = undefined;
queryStore.graphQLErrors = [];
}
this.queryManager.resetErrors(this.queryId);
}

/**
Expand Down Expand Up @@ -307,21 +268,20 @@ export class ObservableQuery<

if (combinedOptions.notifyOnNetworkStatusChange) {
const currentResult = this.getCurrentResult();
const queryInfo = this.queryManager.getQueryStoreValue(this.queryId);
if (queryInfo) {
// If we neglect to update queryInfo.networkStatus here,
// getCurrentResult may return a loading:false result while
// fetchMore is in progress, since getCurrentResult also consults
// queryInfo.networkStatus. Note: setting queryInfo.networkStatus
// to an in-flight status means that QueryInfo#shouldNotify will
// return false while fetchMore is in progress, which is why we
// call this.reobserve() explicitly in the .finally callback after
// fetchMore (below), since the cache write will not automatically
// trigger a notification, even though it does trigger a cache
// broadcast. This is a good thing, because it means we won't see
// intervening query notifications while fetchMore is pending.
queryInfo.networkStatus = NetworkStatus.fetchMore;
}

// If we neglect to update queryInfo.networkStatus here,
// getCurrentResult may return a loading:false result while
// fetchMore is in progress, since getCurrentResult also consults
// queryInfo.networkStatus. Note: setting queryInfo.networkStatus
// to an in-flight status means that QueryInfo#shouldNotify will
// return false while fetchMore is in progress, which is why we
// call this.reobserve() explicitly in the .finally callback after
// fetchMore (below), since the cache write will not automatically
// trigger a notification, even though it does trigger a cache
// broadcast. This is a good thing, because it means we won't see
// intervening query notifications while fetchMore is pending.
this.queryInfo.networkStatus = NetworkStatus.fetchMore;

// Simulate a loading result for the original query with
// networkStatus === NetworkStatus.fetchMore.
this.observer.next!({
Expand Down Expand Up @@ -508,8 +468,15 @@ once, rather than every time you call fetchMore.`);
) => TData,
): void {
const { queryManager } = this;
const previousResult = this.getCurrentQueryResult(false).data;
const newResult = mapFn(previousResult!, {
const { result } = queryManager.cache.diff<TData>({
query: this.options.query,
variables: this.variables,
previousResult: this.lastResult?.data,
returnPartialData: true,
optimistic: false,
});

const newResult = mapFn(result!, {
variables: (this as any).variables,
});

Expand All @@ -524,49 +491,6 @@ once, rather than every time you call fetchMore.`);
}
}

private getCurrentQueryResult(
optimistic: boolean = true,
): {
data?: TData;
partial: boolean;
} {
const { fetchPolicy } = this.options;
const lastData = this.lastResult?.data;
if (fetchPolicy === 'no-cache' ||
fetchPolicy === 'network-only') {
return {
data: lastData,
partial: false,
};
}

let { result, complete } = this.queryManager.cache.diff<TData>({
query: this.options.query,
variables: this.variables,
previousResult: this.lastResult?.data,
returnPartialData: true,
optimistic,
});

if (lastData &&
!this.lastError &&
// If this.options.query has @client(always: true) fields, we
// cannot trust result, since it was read from the cache without
// running local resolvers (and it's too late to run resolvers
// now, since we must return a result synchronously). TODO In the
// future (after Apollo Client 3.0), we should find a way to trust
// this.lastResult in more cases, and read from the cache only in
// cases when no result has been received yet.
this.queryManager.transform(this.options.query).hasForcedResolvers) {
result = lastData;
}

return {
data: (complete || this.options.returnPartialData) ? result : void 0,
partial: !complete,
};
}

public startPolling(pollInterval: number) {
this.getReobserver().updateOptions({ pollInterval });
}
Expand Down Expand Up @@ -659,7 +583,8 @@ once, rather than every time you call fetchMore.`);
);
},
// Avoid polling during SSR and when the query is already in flight.
!queryManager.ssrMode && (() => !queryManager.checkInFlight(queryId)),
!queryManager.ssrMode && (
() => !isNetworkRequestInFlight(this.queryInfo.networkStatus))
);
}

Expand Down
Loading