From 46956c85c9f9def787f9ed191c91f345c73f7f02 Mon Sep 17 00:00:00 2001 From: Jenn Creighton Date: Tue, 9 Mar 2021 13:29:43 -0500 Subject: [PATCH 1/4] Preserve fetchPolicy when notifyOnNetworkStatusChange is set --- src/core/QueryManager.ts | 54 ++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 827b91b7128..76fae1527cb 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -914,7 +914,7 @@ export class QueryManager { const query = this.transform(options.query).document; const variables = this.getVariables(query, options.variables) as TVars; const queryInfo = this.getQuery(queryId); - const oldNetworkStatus = queryInfo.networkStatus; + // const oldNetworkStatus = queryInfo.networkStatus; let { fetchPolicy = "cache-first" as WatchQueryFetchPolicy, @@ -924,26 +924,6 @@ export class QueryManager { context = {}, } = options; - const mightUseNetwork = - fetchPolicy === "cache-first" || - fetchPolicy === "cache-and-network" || - fetchPolicy === "network-only" || - fetchPolicy === "no-cache"; - - if (mightUseNetwork && - notifyOnNetworkStatusChange && - typeof oldNetworkStatus === "number" && - oldNetworkStatus !== networkStatus && - isNetworkRequestInFlight(networkStatus)) { - // In order to force delivery of an incomplete cache result with - // loading:true, we tweak the fetchPolicy to include the cache, and - // pretend that returnPartialData was enabled. - if (fetchPolicy !== "cache-first") { - fetchPolicy = "cache-and-network"; - } - returnPartialData = true; - } - const normalized = Object.assign({}, options, { query, variables, @@ -1038,8 +1018,11 @@ export class QueryManager { errorPolicy, returnPartialData, context, + notifyOnNetworkStatusChange, } = options; + const oldNetworkStatus = queryInfo.networkStatus; + queryInfo.init({ document: query, variables, @@ -1092,6 +1075,13 @@ export class QueryManager { errorPolicy, }); + const shouldNotifyOnNetworkStatusChange = () => ( + notifyOnNetworkStatusChange && + typeof oldNetworkStatus === "number" && + oldNetworkStatus !== networkStatus && + isNetworkRequestInFlight(networkStatus) + ); + switch (fetchPolicy) { default: case "cache-first": { const diff = readCache(); @@ -1109,6 +1099,13 @@ export class QueryManager { ]; } + if (shouldNotifyOnNetworkStatusChange()) { + return [ + resultsFromCache(diff), + resultsFromLink(true), + ]; + } + return [ resultsFromLink(true), ]; @@ -1117,7 +1114,7 @@ export class QueryManager { case "cache-and-network": { const diff = readCache(); - if (diff.complete || returnPartialData) { + if (diff.complete || returnPartialData || shouldNotifyOnNetworkStatusChange()) { return [ resultsFromCache(diff), resultsFromLink(true), @@ -1135,9 +1132,22 @@ export class QueryManager { ]; case "network-only": + if (shouldNotifyOnNetworkStatusChange()) { + const diff = readCache(); + + return [ + resultsFromCache(diff), + resultsFromLink(true), + ]; + } + return [resultsFromLink(true)]; case "no-cache": + if (shouldNotifyOnNetworkStatusChange()) { + return [resultsFromCache(queryInfo.getDiff()), resultsFromLink(false)]; + } + return [resultsFromLink(false)]; case "standby": From 2b1e9c69527837a7351231a607488f7512c03b66 Mon Sep 17 00:00:00 2001 From: Jenn Creighton Date: Tue, 9 Mar 2021 13:30:17 -0500 Subject: [PATCH 2/4] Test that we preserve no-cache with notifyOnNetworkStatusChange set --- src/core/QueryManager.ts | 1 - src/core/__tests__/fetchPolicies.ts | 57 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 76fae1527cb..f41e6f001e2 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -914,7 +914,6 @@ export class QueryManager { const query = this.transform(options.query).document; const variables = this.getVariables(query, options.variables) as TVars; const queryInfo = this.getQuery(queryId); - // const oldNetworkStatus = queryInfo.networkStatus; let { fetchPolicy = "cache-first" as WatchQueryFetchPolicy, diff --git a/src/core/__tests__/fetchPolicies.ts b/src/core/__tests__/fetchPolicies.ts index b67128eb701..3c3106dc2a6 100644 --- a/src/core/__tests__/fetchPolicies.ts +++ b/src/core/__tests__/fetchPolicies.ts @@ -324,6 +324,63 @@ describe('no-cache', () => { }) .then(resolve, reject); }); + + describe('when notifyOnNetworkStatusChange is set', () => { + itAsync('does not save the data to the cache on success', (resolve, reject) => { + let called = 0; + const inspector = new ApolloLink((operation, forward) => { + called++; + return forward(operation).map(result => { + called++; + return result; + }); + }); + + const client = new ApolloClient({ + link: inspector.concat(createLink(reject)), + cache: new InMemoryCache({ addTypename: false }), + }); + + return client.query({ query, fetchPolicy: 'no-cache', notifyOnNetworkStatusChange: true }).then( + () => client.query({ query }).then(actualResult => { + expect(stripSymbols(actualResult.data)).toEqual(result); + // the second query couldn't read anything from the cache + expect(called).toBe(4); + }), + ).then(resolve, reject); + }); + + itAsync('does not save data to the cache on failure', (resolve, reject) => { + let called = 0; + const inspector = new ApolloLink((operation, forward) => { + called++; + return forward(operation).map(result => { + called++; + return result; + }); + }); + + const client = new ApolloClient({ + link: inspector.concat(createFailureLink()), + cache: new InMemoryCache({ addTypename: false }), + }); + + let didFail = false; + return client + .query({ query, fetchPolicy: 'no-cache', notifyOnNetworkStatusChange: true }) + .catch(e => { + expect(e.message).toMatch('query failed'); + didFail = true; + }) + .then(() => client.query({ query }).then(actualResult => { + expect(stripSymbols(actualResult.data)).toEqual(result); + // the first error doesn't call .map on the inspector + expect(called).toBe(3); + expect(didFail).toBe(true); + })) + .then(resolve, reject); + }); + }); }); describe('cache-first', () => { From 0ba32b346096ee93073d5979e909fa3c3e9d51ce Mon Sep 17 00:00:00 2001 From: Jenn Creighton Date: Fri, 12 Mar 2021 16:07:34 -0500 Subject: [PATCH 3/4] Update Changelog.MD --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c18c8d5a647..854f2185e66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ - Maintain serial ordering of `asyncMap` mapping function calls, and prevent potential unhandled `Promise` rejection errors.
[@benjamn](https://github.com/benjamn) in [#7818](https://github.com/apollographql/apollo-client/pull/7818) +- Preserve fetch policy even when `notifyOnNetworkStatusChange` is set
+ [@jcreighton](https://github.com/jcreighton) in [#7761](https://github.com/apollographql/apollo-client/pull/7761) ## Apollo Client 3.3.11 ### Bug fixes From b4cd8fda9fee99d98e5c0636a740d7290a5184e6 Mon Sep 17 00:00:00 2001 From: Jenn Creighton Date: Fri, 12 Mar 2021 16:13:44 -0500 Subject: [PATCH 4/4] Add test for no-cache fetchPolicy + notifyOnNetworkStatusChange + watchQuery --- src/core/__tests__/fetchPolicies.ts | 100 ++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/src/core/__tests__/fetchPolicies.ts b/src/core/__tests__/fetchPolicies.ts index 3c3106dc2a6..f05b56cfe3d 100644 --- a/src/core/__tests__/fetchPolicies.ts +++ b/src/core/__tests__/fetchPolicies.ts @@ -380,6 +380,106 @@ describe('no-cache', () => { })) .then(resolve, reject); }); + + itAsync('gives appropriate networkStatus for watched queries', (resolve, reject) => { + const client = new ApolloClient({ + link: ApolloLink.empty(), + cache: new InMemoryCache(), + resolvers: { + Query: { + hero(_data, args) { + return { + __typename: 'Hero', + ...args, + name: 'Luke Skywalker', + }; + }, + }, + }, + }); + + const observable = client.watchQuery({ + query: gql` + query FetchLuke($id: String) { + hero(id: $id) @client { + id + name + } + } + `, + fetchPolicy: 'no-cache', + variables: { id: '1' }, + notifyOnNetworkStatusChange: true, + }); + + function dataWithId(id: number | string) { + return { + hero: { + __typename: 'Hero', + id: String(id), + name: 'Luke Skywalker', + }, + }; + } + + subscribeAndCount(reject, observable, (count, result) => { + if (count === 1) { + expect(result).toEqual({ + data: dataWithId(1), + loading: false, + networkStatus: NetworkStatus.ready, + }); + expect(client.cache.extract(true)).toEqual({}); + return observable.setVariables({ id: '2' }); + } else if (count === 2) { + expect(result).toEqual({ + data: {}, + loading: true, + networkStatus: NetworkStatus.setVariables, + partial: true, + }); + } else if (count === 3) { + expect(result).toEqual({ + data: dataWithId(2), + loading: false, + networkStatus: NetworkStatus.ready, + }); + expect(client.cache.extract(true)).toEqual({}); + return observable.refetch(); + } else if (count === 4) { + expect(result).toEqual({ + data: dataWithId(2), + loading: true, + networkStatus: NetworkStatus.refetch, + }); + expect(client.cache.extract(true)).toEqual({}); + } else if (count === 5) { + expect(result).toEqual({ + data: dataWithId(2), + loading: false, + networkStatus: NetworkStatus.ready, + }); + expect(client.cache.extract(true)).toEqual({}); + return observable.refetch({ id: '3' }); + } else if (count === 6) { + expect(result).toEqual({ + data: {}, + loading: true, + networkStatus: NetworkStatus.setVariables, + partial: true, + }); + expect(client.cache.extract(true)).toEqual({}); + } else if (count === 7) { + expect(result).toEqual({ + data: dataWithId(3), + loading: false, + networkStatus: NetworkStatus.ready, + }); + expect(client.cache.extract(true)).toEqual({}); + resolve(); + } + }); + }); }); });