diff --git a/.changeset/swift-mails-search.md.md b/.changeset/swift-mails-search.md.md new file mode 100644 index 00000000000..0488199f92b --- /dev/null +++ b/.changeset/swift-mails-search.md.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix a bug where an incoming cache update could prevent future updates from the active link. diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs new file mode 100644 index 00000000000..ca38757c951 --- /dev/null +++ b/.git-blame-ignore-revs @@ -0,0 +1 @@ +78809ce69e2ce8dece8d7cd414756189a23b872f diff --git a/config/bundlesize.ts b/config/bundlesize.ts index 6f142ff6a29..cc877b8f955 100644 --- a/config/bundlesize.ts +++ b/config/bundlesize.ts @@ -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; diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index e48683392e9..b59f5030751 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -692,11 +692,11 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`); private fetch( options: WatchQueryOptions, newNetworkStatus?: NetworkStatus, - ): Concast> { + ) { // 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, @@ -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> = { next: result => { this.reportResult(result, variables); @@ -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) { diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index c14dbe45271..9b9c9f88088 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1159,8 +1159,23 @@ export class QueryManager { // The initial networkStatus for this fetch, most often // NetworkStatus.loading, but also possibly fetchMore, poll, refetch, // or setVariables. - networkStatus = NetworkStatus.loading, + networkStatus?: NetworkStatus, ): Concast> { + return this.fetchConcastWithInfo( + queryId, + options, + networkStatus, + ).concast; + } + + private fetchConcastWithInfo( + queryId: string, + options: WatchQueryOptions, + // The initial networkStatus for this fetch, most often + // NetworkStatus.loading, but also possibly fetchMore, poll, refetch, + // or setVariables. + networkStatus = NetworkStatus.loading + ): ConcastAndInfo { const query = this.transform(options.query).document; const variables = this.getVariables(query, options.variables) as TVars; const queryInfo = this.getQuery(queryId); @@ -1190,7 +1205,7 @@ export class QueryManager { // WatchQueryOptions object. normalized.variables = variables; - const concastSources = this.fetchQueryByPolicy( + const sourcesWithInfo = this.fetchQueryByPolicy( queryInfo, normalized, networkStatus, @@ -1202,13 +1217,13 @@ export class QueryManager { 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, @@ -1220,29 +1235,39 @@ export class QueryManager { setTimeout(() => concast.cancel(reason)); }); - // A Concast can be created either from an Iterable> - // or from a PromiseLike>>, where T in this - // case is ApolloQueryResult. - 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>, + 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({ @@ -1415,8 +1440,8 @@ export class QueryManager { // The initial networkStatus for this fetch, most often // NetworkStatus.loading, but also possibly fetchMore, poll, refetch, // or setVariables. - networkStatus: NetworkStatus, - ): ConcastSourcesArray> { + networkStatus: NetworkStatus + ): SourcesAndInfo { const oldNetworkStatus = queryInfo.networkStatus; queryInfo.init({ @@ -1498,72 +1523,58 @@ export class QueryManager { 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: [] }; } } @@ -1582,3 +1593,15 @@ export class QueryManager { }; } } + +// 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 extends FetchConcastInfo { + sources: ConcastSourcesArray>; +} +interface ConcastAndInfo extends FetchConcastInfo { + concast: Concast>; +} diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index b93e51e497f..17a3a72b6f1 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -22,16 +22,18 @@ import { waitFor } from "@testing-library/react"; export const mockFetchQuery = (queryManager: QueryManager) => { const fetchQueryObservable = queryManager.fetchQueryObservable; + const fetchConcastWithInfo = queryManager['fetchConcastWithInfo']; const fetchQueryByPolicy: QueryManager["fetchQueryByPolicy"] = (queryManager as any) .fetchQueryByPolicy; - const mock = (original: T) => + const mock = (original: T) => jest.fn, Parameters>(function () { return original.apply(queryManager, arguments); }); const mocks = { fetchQueryObservable: mock(fetchQueryObservable), + fetchConcastWithInfo: mock(fetchConcastWithInfo), fetchQueryByPolicy: mock(fetchQueryByPolicy), }; @@ -1000,7 +1002,7 @@ describe("ObservableQuery", () => { expect(fqbpCalls[0][1].fetchPolicy).toEqual("cache-first"); expect(fqbpCalls[1][1].fetchPolicy).toEqual("network-only"); - const fqoCalls = mocks.fetchQueryObservable.mock.calls; + const fqoCalls = mocks.fetchConcastWithInfo.mock.calls; expect(fqoCalls.length).toBe(2); expect(fqoCalls[0][1].fetchPolicy).toEqual("cache-first"); expect(fqoCalls[1][1].fetchPolicy).toEqual("network-only"); @@ -1056,7 +1058,7 @@ describe("ObservableQuery", () => { // FetchPolicy does not switch to cache-first after the first // network request. expect(observable.options.fetchPolicy).toBe("no-cache"); - const fqoCalls = mocks.fetchQueryObservable.mock.calls; + const fqoCalls = mocks.fetchConcastWithInfo.mock.calls; expect(fqoCalls.length).toBe(2); expect(fqoCalls[1][1].fetchPolicy).toBe("no-cache"); @@ -2504,3 +2506,149 @@ describe("ObservableQuery", () => { }); }); }); + +test("regression test for #10587", async () => { + let observers: Record> = {}; + const link = new ApolloLink((operation) => { + return new Observable((observer) => { + observers[operation.operationName] = observer; + }); + }); + + const client = new ApolloClient({ + cache: new InMemoryCache({ + typePolicies: { + SchemaType: { + merge: true, + }, + }, + }).restore({ + ROOT_QUERY: { + __typename: "Query", + schemaType: { __typename: "SchemaType", a: "", b: "" }, + }, + }), + defaultOptions: { + watchQuery: { + fetchPolicy: "cache-and-network", + nextFetchPolicy: "cache-and-network", + }, + }, + link, + }); + + const query1 = client.watchQuery({ + query: gql` + query query1 { + schemaType { + a + } + } + `, + }); + const query2 = client.watchQuery({ + query: gql` + query query2 { + schemaType { + a + b + } + } + `, + }); + const query1Spy = jest.fn(); + const query2Spy = jest.fn(); + + query1.subscribe(query1Spy); + query2.subscribe(query2Spy); + + const finalExpectedCalls = { + query1: [ + [ + { + data: { + schemaType: { + __typename: "SchemaType", + a: "", + }, + }, + loading: true, + networkStatus: 1, + }, + ], + [ + { + data: { + schemaType: { + __typename: "SchemaType", + a: "a", + }, + }, + loading: false, + networkStatus: 7, + }, + ], + ], + query2: [ + [ + { + data: { + schemaType: { + __typename: "SchemaType", + a: "", + b: "", + }, + }, + loading: true, + networkStatus: 1, + }, + ], + [ + { + data: { + schemaType: { + __typename: "SchemaType", + a: "a", + b: "", + }, + }, + // TODO: this should be `true`, but that seems to be a separate bug! + loading: false, + networkStatus: 7, + }, + ], + [ + { + data: { + schemaType: { + __typename: "SchemaType", + a: "a", + b: "b", + }, + }, + loading: false, + networkStatus: 7, + }, + ], + ], + } as const; + + await waitFor(() => expect(query1Spy.mock.calls).toEqual(finalExpectedCalls.query1.slice(0, 1))); + expect(query2Spy.mock.calls).toEqual(finalExpectedCalls.query2.slice(0, 1)); + + observers.query1.next({ + data: { schemaType: { __typename: "SchemaType", a: "a" } }, + }); + observers.query1.complete(); + + await waitFor(() => expect(query1Spy.mock.calls).toEqual(finalExpectedCalls.query1)); + expect(query2Spy.mock.calls).toEqual(finalExpectedCalls.query2.slice(0, 2)); + + observers.query2.next({ + data: { schemaType: { __typename: "SchemaType", a: "a", b: "b" } }, + }); + observers.query2.complete(); + + await waitFor(() => expect(query2Spy.mock.calls).toEqual(finalExpectedCalls.query2)); + expect(query1Spy.mock.calls).toEqual(finalExpectedCalls.query1); +});