Skip to content

Commit

Permalink
Fix a bug where an incoming cache update could prevent future updates…
Browse files Browse the repository at this point in the history
… from the active link.

Co-authored-by: Ben Newman <ben@apollographql.com>
  • Loading branch information
2 people authored and phryneas committed Mar 29, 2023
1 parent 104bf11 commit 8fb9d19
Show file tree
Hide file tree
Showing 6 changed files with 265 additions and 88 deletions.
5 changes: 5 additions & 0 deletions .changeset/swift-mails-search.md.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix a bug where an incoming cache update could prevent future updates from the active link.
1 change: 1 addition & 0 deletions .git-blame-ignore-revs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
78809ce69e2ce8dece8d7cd414756189a23b872f
2 changes: 1 addition & 1 deletion config/bundlesize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { join } from "path";
import { gzipSync } from "zlib";
import bytes from "bytes";

const gzipBundleByteLengthLimit = bytes("32.65KB");
const gzipBundleByteLengthLimit = bytes("32.75KB");
const minFile = join("dist", "apollo-client.min.cjs");
const minPath = join(__dirname, "..", minFile);
const gzipByteLen = gzipSync(readFileSync(minPath)).byteLength;
Expand Down
8 changes: 4 additions & 4 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -692,11 +692,11 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
private fetch(
options: WatchQueryOptions<TVariables, TData>,
newNetworkStatus?: NetworkStatus,
): Concast<ApolloQueryResult<TData>> {
) {
// TODO Make sure we update the networkStatus (and infer fetchVariables)
// before actually committing to the fetch.
this.queryManager.setObservableQuery(this);
return this.queryManager.fetchQueryObservable(
return this.queryManager['fetchConcastWithInfo'](
this.queryId,
options,
newNetworkStatus,
Expand Down Expand Up @@ -835,7 +835,7 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
}

const variables = options.variables && { ...options.variables };
const concast = this.fetch(options, newNetworkStatus);
const { concast, fromLink } = this.fetch(options, newNetworkStatus);
const observer: Observer<ApolloQueryResult<TData>> = {
next: result => {
this.reportResult(result, variables);
Expand All @@ -845,7 +845,7 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
},
};

if (!useDisposableConcast) {
if (!useDisposableConcast && fromLink) {
// We use the {add,remove}Observer methods directly to avoid wrapping
// observer with an unnecessary SubscriptionObserver object.
if (this.concast && this.observer) {
Expand Down
183 changes: 103 additions & 80 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1159,8 +1159,23 @@ export class QueryManager<TStore> {
// The initial networkStatus for this fetch, most often
// NetworkStatus.loading, but also possibly fetchMore, poll, refetch,
// or setVariables.
networkStatus = NetworkStatus.loading,
networkStatus?: NetworkStatus,
): Concast<ApolloQueryResult<TData>> {
return this.fetchConcastWithInfo(
queryId,
options,
networkStatus,
).concast;
}

private fetchConcastWithInfo<TData, TVars extends OperationVariables>(
queryId: string,
options: WatchQueryOptions<TVars, TData>,
// The initial networkStatus for this fetch, most often
// NetworkStatus.loading, but also possibly fetchMore, poll, refetch,
// or setVariables.
networkStatus = NetworkStatus.loading
): ConcastAndInfo<TData> {
const query = this.transform(options.query).document;
const variables = this.getVariables(query, options.variables) as TVars;
const queryInfo = this.getQuery(queryId);
Expand Down Expand Up @@ -1190,7 +1205,7 @@ export class QueryManager<TStore> {
// WatchQueryOptions object.
normalized.variables = variables;

const concastSources = this.fetchQueryByPolicy<TData, TVars>(
const sourcesWithInfo = this.fetchQueryByPolicy<TData, TVars>(
queryInfo,
normalized,
networkStatus,
Expand All @@ -1202,13 +1217,13 @@ export class QueryManager<TStore> {
normalized.fetchPolicy !== "standby" &&
// The "standby" policy currently returns [] from fetchQueryByPolicy, so
// this is another way to detect when nothing was done/fetched.
concastSources.length > 0 &&
sourcesWithInfo.sources.length > 0 &&
queryInfo.observableQuery
) {
queryInfo.observableQuery["applyNextFetchPolicy"]("after-fetch", options);
}

return concastSources;
return sourcesWithInfo;
};

// This cancel function needs to be set before the concast is created,
Expand All @@ -1220,29 +1235,39 @@ export class QueryManager<TStore> {
setTimeout(() => concast.cancel(reason));
});

// A Concast<T> can be created either from an Iterable<Observable<T>>
// or from a PromiseLike<Iterable<Observable<T>>>, where T in this
// case is ApolloQueryResult<TData>.
const concast = new Concast(
// If the query has @export(as: ...) directives, then we need to
// process those directives asynchronously. When there are no
// @export directives (the common case), we deliberately avoid
// wrapping the result of this.fetchQueryByPolicy in a Promise,
// since the timing of result delivery is (unfortunately) important
// for backwards compatibility. TODO This code could be simpler if
// we deprecated and removed LocalState.
this.transform(normalized.query).hasClientExports
? this.localState.addExportedVariables(
normalized.query,
normalized.variables,
normalized.context,
).then(fromVariables)
: fromVariables(normalized.variables!)
);
let concast: Concast<ApolloQueryResult<TData>>,
containsDataFromLink: boolean;
// If the query has @export(as: ...) directives, then we need to
// process those directives asynchronously. When there are no
// @export directives (the common case), we deliberately avoid
// wrapping the result of this.fetchQueryByPolicy in a Promise,
// since the timing of result delivery is (unfortunately) important
// for backwards compatibility. TODO This code could be simpler if
// we deprecated and removed LocalState.
if (this.transform(normalized.query).hasClientExports) {
concast = new Concast(
this.localState
.addExportedVariables(normalized.query, normalized.variables, normalized.context)
.then(fromVariables).then(sourcesWithInfo => sourcesWithInfo.sources),
);
// there is just no way we can synchronously get the *right* value here,
// so we will assume `true`, which is the behaviour before the bug fix in
// #10597. This means that bug is not fixed in that case, and is probably
// un-fixable with reasonable effort for the edge case of @export as
// directives.
containsDataFromLink = true;
} else {
const sourcesWithInfo = fromVariables(normalized.variables);
containsDataFromLink = sourcesWithInfo.fromLink;
concast = new Concast(sourcesWithInfo.sources);
}

concast.promise.then(cleanupCancelFn, cleanupCancelFn);

return concast;
return {
concast,
fromLink: containsDataFromLink,
};
}

public refetchQueries<TResult>({
Expand Down Expand Up @@ -1415,8 +1440,8 @@ export class QueryManager<TStore> {
// The initial networkStatus for this fetch, most often
// NetworkStatus.loading, but also possibly fetchMore, poll, refetch,
// or setVariables.
networkStatus: NetworkStatus,
): ConcastSourcesArray<ApolloQueryResult<TData>> {
networkStatus: NetworkStatus
): SourcesAndInfo<TData> {
const oldNetworkStatus = queryInfo.networkStatus;

queryInfo.init({
Expand Down Expand Up @@ -1498,72 +1523,58 @@ export class QueryManager<TStore> {
isNetworkRequestInFlight(networkStatus);

switch (fetchPolicy) {
default: case "cache-first": {
const diff = readCache();
default: case "cache-first": {
const diff = readCache();

if (diff.complete) {
return [
resultsFromCache(diff, queryInfo.markReady()),
];
}
if (diff.complete) {
return { fromLink: false, sources: [resultsFromCache(diff, queryInfo.markReady())] };
}

if (returnPartialData || shouldNotify) {
return { fromLink: true, sources: [resultsFromCache(diff), resultsFromLink()] };
}

if (returnPartialData || shouldNotify) {
return [
resultsFromCache(diff),
resultsFromLink(),
];
return { fromLink: true, sources: [resultsFromLink()] };
}

return [
resultsFromLink(),
];
}
case "cache-and-network": {
const diff = readCache();

case "cache-and-network": {
const diff = readCache();
if (diff.complete || returnPartialData || shouldNotify) {
return { fromLink: true, sources: [resultsFromCache(diff), resultsFromLink()] };
}

if (diff.complete || returnPartialData || shouldNotify) {
return [
resultsFromCache(diff),
resultsFromLink(),
];
return { fromLink: true, sources: [resultsFromLink()] };
}

return [
resultsFromLink(),
];
}
case "cache-only":
return { fromLink: false, sources: [resultsFromCache(readCache(), queryInfo.markReady())] };

case "cache-only":
return [
resultsFromCache(readCache(), queryInfo.markReady()),
];

case "network-only":
if (shouldNotify) {
return [
resultsFromCache(readCache()),
resultsFromLink(),
];
}
case "network-only":
if (shouldNotify) {
return { fromLink: true, sources: [resultsFromCache(readCache()), resultsFromLink()] };
}

return [resultsFromLink()];

case "no-cache":
if (shouldNotify) {
return [
// Note that queryInfo.getDiff() for no-cache queries does not call
// cache.diff, but instead returns a { complete: false } stub result
// when there is no queryInfo.diff already defined.
resultsFromCache(queryInfo.getDiff()),
resultsFromLink(),
];
}
return { fromLink: true, sources: [resultsFromLink()] };

case "no-cache":
if (shouldNotify) {
return {
fromLink: true,
// Note that queryInfo.getDiff() for no-cache queries does not call
// cache.diff, but instead returns a { complete: false } stub result
// when there is no queryInfo.diff already defined.
sources: [
resultsFromCache(queryInfo.getDiff()),
resultsFromLink(),
],
};
}

return [resultsFromLink()];
return { fromLink: true, sources: [resultsFromLink()] };

case "standby":
return [];
case "standby":
return { fromLink: false, sources: [] };
}
}

Expand All @@ -1582,3 +1593,15 @@ export class QueryManager<TStore> {
};
}
}

// Return types used by fetchQueryByPolicy and other private methods above.
interface FetchConcastInfo {
// Metadata properties that can be returned in addition to the Concast.
fromLink: boolean;
}
interface SourcesAndInfo<TData> extends FetchConcastInfo {
sources: ConcastSourcesArray<ApolloQueryResult<TData>>;
}
interface ConcastAndInfo<TData> extends FetchConcastInfo {
concast: Concast<ApolloQueryResult<TData>>;
}
Loading

0 comments on commit 8fb9d19

Please sign in to comment.