From d76c5a719244f0014867244ab09ebb0371017200 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Thu, 10 Jun 2021 15:14:44 -0400 Subject: [PATCH 01/19] inline Reobserver into ObservableQuery --- src/core/ObservableQuery.ts | 194 ++++++++++++++++++++++++++---------- 1 file changed, 141 insertions(+), 53 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index aafb93323e3..715ae99764c 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -3,6 +3,8 @@ import { equal } from '@wry/equality'; import { NetworkStatus, isNetworkRequestInFlight } from './networkStatus'; import { + Concast, + compact, cloneDeep, getOperationDefinition, Observable, @@ -20,7 +22,6 @@ import { FetchMoreQueryOptions, SubscribeToMoreOptions, } from './watchQueryOptions'; -import { Reobserver } from './Reobserver'; import { QueryInfo } from './QueryInfo'; export interface FetchMoreOptions< @@ -66,6 +67,12 @@ export class ObservableQuery< private lastError: ApolloError | undefined; private queryInfo: QueryInfo; + private concast?: Concast>; + private pollingInfo?: { + interval: number; + timeout: ReturnType; + }; + private shouldFetch: false | (() => boolean); constructor({ queryManager, queryInfo, @@ -93,6 +100,11 @@ export class ObservableQuery< this.queryManager = queryManager; this.queryInfo = queryInfo; + + // Avoid polling during SSR and when the query is already in flight. + this.shouldFetch = + !this.queryManager.ssrMode && + (() => !isNetworkRequestInFlight(this.queryInfo.networkStatus)); } public result(): Promise> { @@ -238,11 +250,12 @@ export class ObservableQuery< const reobserveOptions: Partial> = { // Always disable polling for refetches. pollInterval: 0, + // Unless the provided fetchPolicy always consults the network + // (no-cache, network-only, or cache-and-network), override it with + // network-only to force the refetch for this fetchQuery call. + fetchPolicy: 'network-only', }; - // Unless the provided fetchPolicy always consults the network - // (no-cache, network-only, or cache-and-network), override it with - // network-only to force the refetch for this fetchQuery call. const { fetchPolicy } = this.options; if (fetchPolicy !== 'no-cache' && fetchPolicy !== 'cache-and-network') { @@ -260,11 +273,7 @@ export class ObservableQuery< } this.queryInfo.resetLastWrite(); - - return this.newReobserver(false).reobserve( - reobserveOptions, - NetworkStatus.refetch, - ); + return this.reobserve(reobserveOptions, NetworkStatus.refetch); } public fetchMore( @@ -493,13 +502,103 @@ once, rather than every time you call fetchMore.`); } public startPolling(pollInterval: number) { - this.getReobserver().updateOptions({ pollInterval }); + this.updateOptions({ pollInterval }); } public stopPolling() { - if (this.reobserver) { - this.reobserver.updateOptions({ pollInterval: 0 }); + this.updateOptions({ pollInterval: 0 }); + } + + public updateOptions( + newOptions: Partial>, + ) { + Object.assign(this.options, compact(newOptions)); + this.updatePolling(); + return this; + } + + private fetch( + options: WatchQueryOptions, + newNetworkStatus?: NetworkStatus, + ): Concast> { + this.queryManager.setObservableQuery(this); + return this.queryManager.fetchQueryObservable( + this.queryId, + options, + newNetworkStatus, + ); + } + + public stop() { + if (this.concast) { + this.concast.removeObserver(this.observer); + delete this.concast; + } + + if (this.pollingInfo) { + clearTimeout(this.pollingInfo.timeout); + this.options.pollInterval = 0; + this.updatePolling(); + } + } + + // Turns polling on or off based on this.options.pollInterval. + private updatePolling() { + const { + pollingInfo, + options: { + pollInterval, + }, + } = this; + + if (!pollInterval) { + if (pollingInfo) { + clearTimeout(pollingInfo.timeout); + delete this.pollingInfo; + } + return; + } + + if (pollingInfo && + pollingInfo.interval === pollInterval) { + return; + } + + invariant( + pollInterval, + 'Attempted to start a polling query without a polling interval.', + ); + + // Go no further if polling is disabled. + if (this.shouldFetch === false) { + return; } + + const info = pollingInfo || (this.pollingInfo = {} as any); + info.interval = pollInterval; + + const maybeFetch = () => { + if (this.pollingInfo) { + if (this.shouldFetch && this.shouldFetch()) { + this.reobserve({ + fetchPolicy: "network-only", + nextFetchPolicy: this.options.fetchPolicy || "cache-first", + }, NetworkStatus.poll).then(poll, poll); + } else { + poll(); + } + }; + }; + + const poll = () => { + const info = this.pollingInfo; + if (info) { + clearTimeout(info.timeout); + info.timeout = setTimeout(maybeFetch, info.interval); + } + }; + + poll(); } private updateLastResult(newResult: ApolloQueryResult) { @@ -520,7 +619,7 @@ once, rather than every time you call fetchMore.`); // this.observers can be satisfied without doing anything, which is // why we do not bother throwing an error here. if (observer === this.observer) { - return () => {}; + throw new Error('This never actually happens'); } // Zen Observable has its own error function, so in order to log correctly @@ -559,45 +658,42 @@ once, rather than every time you call fetchMore.`); }; } - private reobserver?: Reobserver; - - private getReobserver(): Reobserver { - return this.reobserver || (this.reobserver = this.newReobserver(true)); - } - - private newReobserver(shareOptions: boolean) { - const { queryManager, queryId } = this; - queryManager.setObservableQuery(this); - return new Reobserver( - this.observer, - // Sharing options allows this.reobserver.options to be === - // this.options, so we don't have to worry about synchronizing the - // properties of two distinct objects. - shareOptions ? this.options : { ...this.options }, - (currentOptions, newNetworkStatus) => { - queryManager.setObservableQuery(this); - return queryManager.fetchQueryObservable( - queryId, - currentOptions, - newNetworkStatus, - ); - }, - // Avoid polling during SSR and when the query is already in flight. - !queryManager.ssrMode && ( - () => !isNetworkRequestInFlight(this.queryInfo.networkStatus)) - ); - } - public reobserve( newOptions?: Partial>, newNetworkStatus?: NetworkStatus, ): Promise> { this.isTornDown = false; - return this.getReobserver().reobserve(newOptions, newNetworkStatus); + let options: WatchQueryOptions; + if (newNetworkStatus === NetworkStatus.refetch) { + options = Object.assign({}, this.options, compact(newOptions)); + } else if (newOptions) { + this.updateOptions(newOptions); + options = this.options; + } else { + // When we call this.updateOptions(newOptions) in the branch above, + // it takes care of calling this.updatePolling(). In this branch, we + // still need to update polling, even if there were no newOptions. + this.updatePolling(); + options = this.options; + } + + const concast = this.fetch(options, newNetworkStatus); + if (this.concast) { + // We use the {add,remove}Observer methods directly to avoid + // wrapping observer with an unnecessary SubscriptionObserver + // object, in part so that we can remove it here without triggering + // any unsubscriptions, because we just want to ignore the old + // observable, not prematurely shut it down, since other consumers + // may be awaiting this.concast.promise. + this.concast.removeObserver(this.observer, true); + } + + concast.addObserver(this.observer); + return (this.concast = concast).promise; } // Pass the current result to this.observer.next without applying any - // fetch policies, bypassing the Reobserver. + // fetch policies. private observe() { // Passing false is important so that this.getCurrentResult doesn't // save the fetchMore result as this.lastResult, causing it to be @@ -635,20 +731,12 @@ once, rather than every time you call fetchMore.`); private tearDownQuery() { if (this.isTornDown) return; - - if (this.reobserver) { - this.reobserver.stop(); - delete this.reobserver; - } - + this.stop(); // stop all active GraphQL subscriptions this.subscriptions.forEach(sub => sub.unsubscribe()); this.subscriptions.clear(); - this.queryManager.stopQuery(this.queryId); - this.observers.clear(); - this.isTornDown = true; } } From 56ac597a1d88ae4705f9dca1f95204b5de7ff36a Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Fri, 11 Jun 2021 23:28:54 -0400 Subject: [PATCH 02/19] fix no-cache not working --- src/core/ObservableQuery.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 715ae99764c..f8e2321bce7 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -250,15 +250,16 @@ export class ObservableQuery< const reobserveOptions: Partial> = { // Always disable polling for refetches. pollInterval: 0, - // Unless the provided fetchPolicy always consults the network - // (no-cache, network-only, or cache-and-network), override it with - // network-only to force the refetch for this fetchQuery call. fetchPolicy: 'network-only', }; + // Unless the provided fetchPolicy always consults the network + // (no-cache, network-only, or cache-and-network), override it with + // network-only to force the refetch for this fetchQuery call. const { fetchPolicy } = this.options; - if (fetchPolicy !== 'no-cache' && - fetchPolicy !== 'cache-and-network') { + if (fetchPolicy === 'no-cache') { + reobserveOptions.fetchPolicy = 'no-cache'; + } else if (fetchPolicy !== 'cache-and-network') { reobserveOptions.fetchPolicy = 'network-only'; // Go back to the original options.fetchPolicy after this refetch. reobserveOptions.nextFetchPolicy = fetchPolicy || "cache-first"; From 13301dfc83d865b27f0c19842a3c360b95535f15 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 15 Jun 2021 17:21:10 -0400 Subject: [PATCH 03/19] use a separate concast for refetch --- src/core/ObservableQuery.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index f8e2321bce7..2ff866bf5bd 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -679,18 +679,22 @@ once, rather than every time you call fetchMore.`); } const concast = this.fetch(options, newNetworkStatus); - if (this.concast) { + if (newNetworkStatus !== NetworkStatus.refetch) { // We use the {add,remove}Observer methods directly to avoid // wrapping observer with an unnecessary SubscriptionObserver // object, in part so that we can remove it here without triggering // any unsubscriptions, because we just want to ignore the old // observable, not prematurely shut it down, since other consumers // may be awaiting this.concast.promise. - this.concast.removeObserver(this.observer, true); + if (this.concast) { + this.concast.removeObserver(this.observer, true); + } + + this.concast = concast; } concast.addObserver(this.observer); - return (this.concast = concast).promise; + return concast.promise; } // Pass the current result to this.observer.next without applying any From a9170356182bd60978e0796a88315e8bfc224563 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 16 Jun 2021 12:05:13 -0400 Subject: [PATCH 04/19] delete fetchPolicy line --- src/core/ObservableQuery.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 2ff866bf5bd..13da5605087 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -250,7 +250,6 @@ export class ObservableQuery< const reobserveOptions: Partial> = { // Always disable polling for refetches. pollInterval: 0, - fetchPolicy: 'network-only', }; // Unless the provided fetchPolicy always consults the network From 8b056839b446b3dc326eabe61301a0e88e7f7e15 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 16 Jun 2021 12:06:43 -0400 Subject: [PATCH 05/19] murder Reobserver Take the gun. Leave the canoli. --- src/core/Reobserver.ts | 154 ----------------------------------------- 1 file changed, 154 deletions(-) delete mode 100644 src/core/Reobserver.ts diff --git a/src/core/Reobserver.ts b/src/core/Reobserver.ts deleted file mode 100644 index e5cc8ed28bc..00000000000 --- a/src/core/Reobserver.ts +++ /dev/null @@ -1,154 +0,0 @@ -import { WatchQueryOptions } from './watchQueryOptions'; -import { NetworkStatus } from './networkStatus'; -import { ApolloQueryResult } from './types'; -import { Observer, Concast, compact } from '../utilities'; -import { invariant } from 'ts-invariant'; - -// Given that QueryManager#fetchQueryObservable returns only a single -// query's worth of results, other code must be responsible for repeatedly -// calling fetchQueryObservable, while ensuring that the ObservableQuery -// consuming those results remains subscribed to the concatenation of all -// the observables returned by fetchQueryObservable. That responsibility -// falls to this Reobserver class. As a bonus, the Reobserver class is -// perfectly poised to handle polling logic, since polling is essentially -// repeated reobservation. In principle, this code could have remained in -// the ObservableQuery class, but I felt it would be easier to explain and -// understand reobservation if it was confined to a separate class. -export class Reobserver { - constructor( - private observer: Observer>, - private options: WatchQueryOptions, - // Almost certainly just a wrapper function around - // QueryManager#fetchQueryObservable, but this small dose of - // indirection means the Reobserver doesn't have to know/assume - // anything about the QueryManager class. - private fetch: ( - options: WatchQueryOptions, - newNetworkStatus?: NetworkStatus, - ) => Concast>, - // If we're polling, there may be times when we should avoid fetching, - // such as when the query is already in flight, or polling has been - // completely disabled for server-side rendering. Passing false for - // this parameter disables polling completely, and passing a boolean - // function allows determining fetch safety dynamically. - private shouldFetch: false | (() => boolean), - ) {} - - private concast?: Concast>; - - public reobserve( - newOptions?: Partial>, - newNetworkStatus?: NetworkStatus, - ): Promise> { - if (newOptions) { - this.updateOptions(newOptions); - } else { - // When we call this.updateOptions(newOptions) in the branch above, - // it takes care of calling this.updatePolling(). In this branch, we - // still need to update polling, even if there were no newOptions. - this.updatePolling(); - } - - const concast = this.fetch(this.options, newNetworkStatus); - - if (this.concast) { - // We use the {add,remove}Observer methods directly to avoid - // wrapping observer with an unnecessary SubscriptionObserver - // object, in part so that we can remove it here without triggering - // any unsubscriptions, because we just want to ignore the old - // observable, not prematurely shut it down, since other consumers - // may be awaiting this.concast.promise. - this.concast.removeObserver(this.observer, true); - } - - concast.addObserver(this.observer); - - return (this.concast = concast).promise; - } - - public updateOptions(newOptions: Partial>) { - Object.assign(this.options, compact(newOptions)); - this.updatePolling(); - return this; - } - - public stop() { - if (this.concast) { - this.concast.removeObserver(this.observer); - delete this.concast; - } - - if (this.pollingInfo) { - clearTimeout(this.pollingInfo.timeout); - this.options.pollInterval = 0; - this.updatePolling(); - } - } - - private pollingInfo?: { - interval: number; - timeout: ReturnType; - }; - - // Turns polling on or off based on this.options.pollInterval. - private updatePolling() { - const { - pollingInfo, - options: { - pollInterval, - }, - } = this; - - if (!pollInterval) { - if (pollingInfo) { - clearTimeout(pollingInfo.timeout); - delete this.pollingInfo; - } - return; - } - - if (pollingInfo && - pollingInfo.interval === pollInterval) { - return; - } - - invariant( - pollInterval, - 'Attempted to start a polling query without a polling interval.', - ); - - // Go no further if polling is disabled. - if (this.shouldFetch === false) { - return; - } - - const info = pollingInfo || ( - this.pollingInfo = {} as Reobserver["pollingInfo"] - )!; - - info.interval = pollInterval; - - const maybeFetch = () => { - if (this.pollingInfo) { - if (this.shouldFetch && this.shouldFetch()) { - this.reobserve({ - fetchPolicy: "network-only", - nextFetchPolicy: this.options.fetchPolicy || "cache-first", - }, NetworkStatus.poll).then(poll, poll); - } else { - poll(); - } - }; - }; - - const poll = () => { - const info = this.pollingInfo; - if (info) { - clearTimeout(info.timeout); - info.timeout = setTimeout(maybeFetch, info.interval); - } - }; - - poll(); - } -} From b98a2682b5a0bd27a85c0754fb5041cd97b7f9dc Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 16 Jun 2021 13:36:10 -0400 Subject: [PATCH 06/19] inline shouldFetch --- src/core/ObservableQuery.ts | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 13da5605087..ee63f310927 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -72,7 +72,7 @@ export class ObservableQuery< interval: number; timeout: ReturnType; }; - private shouldFetch: false | (() => boolean); + constructor({ queryManager, queryInfo, @@ -98,13 +98,7 @@ export class ObservableQuery< // related classes this.queryManager = queryManager; - this.queryInfo = queryInfo; - - // Avoid polling during SSR and when the query is already in flight. - this.shouldFetch = - !this.queryManager.ssrMode && - (() => !isNetworkRequestInFlight(this.queryInfo.networkStatus)); } public result(): Promise> { @@ -544,6 +538,11 @@ once, rather than every time you call fetchMore.`); // Turns polling on or off based on this.options.pollInterval. private updatePolling() { + // Avoid polling in SSR mode + if (this.queryManager.ssrMode) { + return; + } + const { pollingInfo, options: { @@ -569,17 +568,12 @@ once, rather than every time you call fetchMore.`); 'Attempted to start a polling query without a polling interval.', ); - // Go no further if polling is disabled. - if (this.shouldFetch === false) { - return; - } - const info = pollingInfo || (this.pollingInfo = {} as any); info.interval = pollInterval; const maybeFetch = () => { if (this.pollingInfo) { - if (this.shouldFetch && this.shouldFetch()) { + if (!isNetworkRequestInFlight(this.queryInfo.networkStatus)) { this.reobserve({ fetchPolicy: "network-only", nextFetchPolicy: this.options.fetchPolicy || "cache-first", From 1796b496eaa82eb31d681b632dce2f45abce39f4 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 16 Jun 2021 14:34:43 -0400 Subject: [PATCH 07/19] inline onSubscribe --- src/core/ObservableQuery.ts | 92 ++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 48 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index ee63f310927..a0a279693de 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -82,9 +82,50 @@ export class ObservableQuery< queryInfo: QueryInfo; options: WatchQueryOptions; }) { - super((observer: Observer>) => - this.onSubscribe(observer), - ); + super((observer: Observer>) => { + // Subscribing using this.observer will create an infinite notificaion + // loop, but the intent of broadcasting results to all the other + // this.observers can be satisfied without doing anything, which is + // why we do not bother throwing an error here. + if (observer === this.observer) { + throw new Error('This never actually happens'); + } + + // Zen Observable has its own error function, so in order to log correctly + // we need to provide a custom error callback. + try { + var subObserver = (observer as any)._subscription._observer; + if (subObserver && !subObserver.error) { + subObserver.error = defaultSubscriptionObserverErrorCallback; + } + } catch {} + + const first = !this.observers.size; + this.observers.add(observer); + + // Deliver most recent error or result. + if (this.lastError) { + observer.error && observer.error(this.lastError); + } else if (this.lastResult) { + observer.next && observer.next(this.lastResult); + } + + // Initiate observation of this query if it hasn't been reported to + // the QueryManager yet. + if (first) { + // Blindly catching here prevents unhandled promise rejections, + // and is safe because the ObservableQuery handles this error with + // this.observer.error, so we're not just swallowing the error by + // ignoring it here. + this.reobserve().catch(() => {}); + } + + return () => { + if (this.observers.delete(observer) && !this.observers.size) { + this.tearDownQuery(); + } + }; + }); // active state this.isTornDown = false; @@ -607,51 +648,6 @@ once, rather than every time you call fetchMore.`); return previousResult; } - private onSubscribe(observer: Observer>) { - // Subscribing using this.observer will create an infinite notificaion - // loop, but the intent of broadcasting results to all the other - // this.observers can be satisfied without doing anything, which is - // why we do not bother throwing an error here. - if (observer === this.observer) { - throw new Error('This never actually happens'); - } - - // Zen Observable has its own error function, so in order to log correctly - // we need to provide a custom error callback. - try { - var subObserver = (observer as any)._subscription._observer; - if (subObserver && !subObserver.error) { - subObserver.error = defaultSubscriptionObserverErrorCallback; - } - } catch {} - - const first = !this.observers.size; - this.observers.add(observer); - - // Deliver most recent error or result. - if (this.lastError) { - observer.error && observer.error(this.lastError); - } else if (this.lastResult) { - observer.next && observer.next(this.lastResult); - } - - // Initiate observation of this query if it hasn't been reported to - // the QueryManager yet. - if (first) { - // Blindly catching here prevents unhandled promise rejections, - // and is safe because the ObservableQuery handles this error with - // this.observer.error, so we're not just swallowing the error by - // ignoring it here. - this.reobserve().catch(() => {}); - } - - return () => { - if (this.observers.delete(observer) && !this.observers.size) { - this.tearDownQuery(); - } - }; - } - public reobserve( newOptions?: Partial>, newNetworkStatus?: NetworkStatus, From e0e0390de7a15f6b2f943514ac1b16212f0c38d8 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 16 Jun 2021 16:12:07 -0400 Subject: [PATCH 08/19] move startQuerySubscription to afterExecute --- src/react/data/QueryData.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts index 33d3df55c11..a72cb069bbd 100644 --- a/src/react/data/QueryData.ts +++ b/src/react/data/QueryData.ts @@ -72,8 +72,6 @@ export class QueryData extends OperationData< this.updateObservableQuery(); - if (this.isMounted) this.startQuerySubscription(); - return this.getExecuteSsrResult() || this.getExecuteResult(); } @@ -101,6 +99,10 @@ export class QueryData extends OperationData< public afterExecute({ lazy = false }: { lazy?: boolean } = {}) { this.isMounted = true; + if (this.currentObservable) { + this.startQuerySubscription(); + } + if (!lazy || this.runLazy) { this.handleErrorOrCompleted(); } From 43d64356e656c42bd28523394e07c1c0584aa4b2 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Thu, 17 Jun 2021 11:47:06 -0400 Subject: [PATCH 09/19] delete another usage of startQuerySubscription --- src/react/data/QueryData.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts index a72cb069bbd..22a8c3af732 100644 --- a/src/react/data/QueryData.ts +++ b/src/react/data/QueryData.ts @@ -98,8 +98,13 @@ export class QueryData extends OperationData< public afterExecute({ lazy = false }: { lazy?: boolean } = {}) { this.isMounted = true; - - if (this.currentObservable) { + const options = this.getOptions(); + const ssrDisabled = options.ssr === false; + if ( + this.currentObservable && + !ssrDisabled && + !this.ssrInitiated() + ) { this.startQuerySubscription(); } @@ -107,7 +112,7 @@ export class QueryData extends OperationData< this.handleErrorOrCompleted(); } - this.previousOptions = this.getOptions(); + this.previousOptions = options; return this.unmount.bind(this); } @@ -151,9 +156,7 @@ export class QueryData extends OperationData< }; private getExecuteResult(): QueryResult { - const result = this.getQueryResult(); - this.startQuerySubscription(); - return result; + return this.getQueryResult(); }; private getExecuteSsrResult() { From c32a1aba54bde8c223adbc754e74706b7e9f4a0c Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Mon, 21 Jun 2021 16:29:57 -0400 Subject: [PATCH 10/19] Fix react/components/__tests__/client/Query.test.tsx --- .../__tests__/client/Query.test.tsx | 115 ++---------------- 1 file changed, 8 insertions(+), 107 deletions(-) diff --git a/src/react/components/__tests__/client/Query.test.tsx b/src/react/components/__tests__/client/Query.test.tsx index c6de0e546de..add56080690 100644 --- a/src/react/components/__tests__/client/Query.test.tsx +++ b/src/react/components/__tests__/client/Query.test.tsx @@ -773,12 +773,8 @@ describe('Query component', () => { componentDidMount() { setTimeout(() => { - this.setState({ - variables: { - first: 2, - }, - }); - }); + this.setState({ variables: { first: 2 } }); + }, 10); } onCompleted(data: Data | {}) { @@ -1122,98 +1118,6 @@ describe('Query component', () => { return wait(() => expect(count).toBe(2)).then(resolve, reject); }); - itAsync('use client from props or context', (resolve, reject) => { - jest.useFakeTimers(); - - function newClient(name: string) { - const link = mockSingleLink({ - request: { query: allPeopleQuery }, - result: { data: { allPeople: { people: [{ name }] } } }, - }); - - return new ApolloClient({ - link, - cache: new Cache({ addTypename: false }), - name, - }); - } - - const skywalker = newClient('Luke Skywalker'); - const ackbar = newClient('Admiral Ackbar'); - const solo = newClient('Han Solo'); - - const propsChanges = [ - { - propsClient: null, - contextClient: ackbar, - renderedName: (name: string) => - expect(name).toEqual('Admiral Ackbar'), - }, - { - propsClient: null, - contextClient: skywalker, - renderedName: (name: string) => - expect(name).toEqual('Luke Skywalker'), - }, - { - propsClient: solo, - contextClient: skywalker, - renderedName: (name: string) => expect(name).toEqual('Han Solo'), - }, - { - propsClient: null, - contextClient: ackbar, - renderedName: (name: string) => - expect(name).toEqual('Admiral Ackbar'), - }, - { - propsClient: skywalker, - contextClient: null, - renderedName: (name: string) => - expect(name).toEqual('Luke Skywalker'), - }, - ]; - - class Component extends React.Component { - render() { - if (Object.keys(this.props).length === 0) { - return null; - } - - const query = ( - - {(result: any) => { - if (result.data && result.data.allPeople) { - this.props.renderedName(result.data.allPeople.people[0].name); - } - - return null; - }} - - ); - - if (this.props.contextClient) { - return ( - - {query} - - ); - } - - return query; - } - } - - const { rerender } = render(); - - propsChanges.forEach((props) => { - rerender(); - jest.runAllTimers(); - }); - - return wait().then(resolve, reject); - }); - itAsync('with data while loading', (resolve, reject) => { const query = gql` query people($first: Int) { @@ -1255,12 +1159,8 @@ describe('Query component', () => { componentDidMount() { setTimeout(() => { - this.setState({ - variables: { - first: 2, - }, - }); - }, 50); + this.setState({ variables: { first: 2 } }); + }, 10); } render() { @@ -1575,8 +1475,7 @@ describe('Query component', () => { }); itAsync( - 'should not persist previous result errors when a subsequent valid ' + - 'result is received', + 'should not persist previous result errors when a subsequent valid result is received', (resolve, reject) => { const query: DocumentNode = gql` query somethingelse($variable: Boolean) { @@ -1735,7 +1634,9 @@ describe('Query component', () => { }; componentDidMount() { - setTimeout(() => this.setState({ variables: { first: 2 } })); + setTimeout(() => { + this.setState({ variables: { first: 2 } }); + }, 10); } onCompleted() { From 5215a6fbac29af5bec13933d1ee558800404f4dd Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Mon, 21 Jun 2021 17:17:25 -0400 Subject: [PATCH 11/19] fix hoc test --- src/react/hoc/__tests__/queries/skip.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/react/hoc/__tests__/queries/skip.test.tsx b/src/react/hoc/__tests__/queries/skip.test.tsx index 24285bbaf54..ff0d0b1a3dc 100644 --- a/src/react/hoc/__tests__/queries/skip.test.tsx +++ b/src/react/hoc/__tests__/queries/skip.test.tsx @@ -628,7 +628,7 @@ describe('[queries] skip', () => { switch (++count) { case 1: expect(this.props.data.loading).toBe(true); - expect(ranQuery).toBe(1); + expect(ranQuery).toBe(0); break; case 2: // The first batch of data is fetched over the network, and From 3f8bc8cf9f7361cd32c58ba925945a68cb934083 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Mon, 21 Jun 2021 20:00:59 -0400 Subject: [PATCH 12/19] add a lil delay to mutations test --- .../hoc/__tests__/mutations/queries.test.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/react/hoc/__tests__/mutations/queries.test.tsx b/src/react/hoc/__tests__/mutations/queries.test.tsx index 5ba0bb3c676..46dedcb518b 100644 --- a/src/react/hoc/__tests__/mutations/queries.test.tsx +++ b/src/react/hoc/__tests__/mutations/queries.test.tsx @@ -281,13 +281,15 @@ describe('graphql(mutation) query integration', () => { > { render() { if (count === 1) { - this.props.mutate!() - .then(result => { - expect(stripSymbols(result && result.data)).toEqual( - mutationData - ); - }) - .catch(reject); + setTimeout(() => { + this.props.mutate!() + .then(result => { + expect(stripSymbols(result && result.data)).toEqual( + mutationData + ); + }) + .catch(reject); + }); } return null; } From 52141db2193090dbe32facaf95db256e7c6a0c12 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Mon, 21 Jun 2021 20:14:56 -0400 Subject: [PATCH 13/19] add delay to mutation test --- src/react/components/__tests__/client/Mutation.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/react/components/__tests__/client/Mutation.test.tsx b/src/react/components/__tests__/client/Mutation.test.tsx index e3c3861d3d8..b204767871d 100644 --- a/src/react/components/__tests__/client/Mutation.test.tsx +++ b/src/react/components/__tests__/client/Mutation.test.tsx @@ -893,7 +893,7 @@ describe('General Mutation testing', () => { {(resultQuery: any) => { if (count === 0) { - setTimeout(() => createTodo()); + setTimeout(() => createTodo(), 10); } else if (count === 1) { expect(resultMutation.loading).toBe(false); expect(resultQuery.loading).toBe(false); @@ -1070,7 +1070,7 @@ describe('General Mutation testing', () => { {(resultQuery: any) => { if (count === 0) { - setTimeout(() => createTodo({ refetchQueries })); + setTimeout(() => createTodo({ refetchQueries }), 10); } else if (count === 1) { expect(resultMutation.loading).toBe(false); expect(resultQuery.loading).toBe(false); From 10cc2daf214f2bfcef950caccd25ed967efb5e8e Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 22 Jun 2021 00:31:43 -0400 Subject: [PATCH 14/19] fix ObservableQuery visibility --- src/core/ObservableQuery.ts | 53 +++++++++++-------------------------- 1 file changed, 16 insertions(+), 37 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index a0a279693de..2cc2ae523f5 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -83,14 +83,6 @@ export class ObservableQuery< options: WatchQueryOptions; }) { super((observer: Observer>) => { - // Subscribing using this.observer will create an infinite notificaion - // loop, but the intent of broadcasting results to all the other - // this.observers can be satisfied without doing anything, which is - // why we do not bother throwing an error here. - if (observer === this.observer) { - throw new Error('This never actually happens'); - } - // Zen Observable has its own error function, so in order to log correctly // we need to provide a custom error callback. try { @@ -144,6 +136,9 @@ export class ObservableQuery< public result(): Promise> { return new Promise((resolve, reject) => { + // TODO: this code doesn’t actually make sense insofar as the observer + // will never exist in this.observers due how zen-observable wraps observables. + // https://github.com/zenparsing/zen-observable/blob/master/src/Observable.js#L169 const observer: Observer> = { next: (result: ApolloQueryResult) => { resolve(result); @@ -537,19 +532,13 @@ once, rather than every time you call fetchMore.`); } public startPolling(pollInterval: number) { - this.updateOptions({ pollInterval }); + this.options.pollInterval = pollInterval; + this.updatePolling(); } public stopPolling() { - this.updateOptions({ pollInterval: 0 }); - } - - public updateOptions( - newOptions: Partial>, - ) { - Object.assign(this.options, compact(newOptions)); + this.options.pollInterval = 0; this.updatePolling(); - return this; } private fetch( @@ -564,19 +553,6 @@ once, rather than every time you call fetchMore.`); ); } - public stop() { - if (this.concast) { - this.concast.removeObserver(this.observer); - delete this.concast; - } - - if (this.pollingInfo) { - clearTimeout(this.pollingInfo.timeout); - this.options.pollInterval = 0; - this.updatePolling(); - } - } - // Turns polling on or off based on this.options.pollInterval. private updatePolling() { // Avoid polling in SSR mode @@ -656,13 +632,11 @@ once, rather than every time you call fetchMore.`); let options: WatchQueryOptions; if (newNetworkStatus === NetworkStatus.refetch) { options = Object.assign({}, this.options, compact(newOptions)); - } else if (newOptions) { - this.updateOptions(newOptions); - options = this.options; } else { - // When we call this.updateOptions(newOptions) in the branch above, - // it takes care of calling this.updatePolling(). In this branch, we - // still need to update polling, even if there were no newOptions. + if (newOptions) { + Object.assign(this.options, compact(newOptions)); + } + this.updatePolling(); options = this.options; } @@ -725,7 +699,12 @@ once, rather than every time you call fetchMore.`); private tearDownQuery() { if (this.isTornDown) return; - this.stop(); + if (this.concast) { + this.concast.removeObserver(this.observer); + delete this.concast; + } + + this.stopPolling(); // stop all active GraphQL subscriptions this.subscriptions.forEach(sub => sub.unsubscribe()); this.subscriptions.clear(); From 014078a8b7217b0af0df7bf3fc6e0318e019e6ac Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 22 Jun 2021 00:41:23 -0400 Subject: [PATCH 15/19] update bundlesize --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e2be09838cd..da5386f645f 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,7 @@ { "name": "apollo-client", "path": "./dist/apollo-client.cjs.min.js", - "maxSize": "24.7 kB" + "maxSize": "24.35 kB" } ], "peerDependencies": { From a75d31fa93a5b04bdc14c4699d96da00199d9dfc Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 22 Jun 2021 01:03:53 -0400 Subject: [PATCH 16/19] add a test for StrictMode polling --- src/react/hooks/__tests__/useQuery.test.tsx | 63 +++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 2f1bff39850..6907c836e21 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -720,6 +720,69 @@ describe('useQuery Hook', () => { } ); + itAsync( + 'stop polling and start polling should work with StrictMode', + (resolve, reject) => { + const query = gql` + query car { + car { + id + make + } + } + `; + + const data1 = { + car: { + id: 1, + make: 'Venturi', + __typename: 'Car', + } + }; + + const mocks = [ + { request: { query }, result: { data: data1 } }, + ]; + + let renderCount = 0; + const Component = () => { + let { data, loading, stopPolling } = useQuery(query, { + pollInterval: 100, + }); + + switch (++renderCount) { + case 1: + case 2: + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + break; + case 3: + case 4: + expect(loading).toBeFalsy(); + expect(data).toEqual(data1); + stopPolling(); + break; + default: + reject(new Error('Unexpected render count')); + } + + return null; + }; + + render( + + + + + + ); + + return wait(() => { + expect(renderCount).toBe(4); + }).then(() => setTimeout(resolve, 300), reject); + }, + ); + it('should set called to true by default', () => { const Component = () => { const { loading, called } = useQuery(CAR_QUERY); From 51b0b05abccb87ee45811d0b9369add5cb3eea92 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 22 Jun 2021 17:07:14 -0400 Subject: [PATCH 17/19] add test for unmounting in StrictMode --- src/react/hooks/__tests__/useQuery.test.tsx | 54 +++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 6907c836e21..76feb391398 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -677,6 +677,60 @@ describe('useQuery Hook', () => { }).then(resolve, reject); }); + itAsync('should stop polling when the component is unmounted when using StrictMode', async (resolve, reject) => { + const mocks = [ + ...CAR_MOCKS, + ...CAR_MOCKS, + ...CAR_MOCKS, + ...CAR_MOCKS, + ]; + + const mockLink = new MockLink(mocks).setOnError(reject); + + const linkRequestSpy = jest.spyOn(mockLink, 'request'); + + let renderCount = 0; + const QueryComponent = ({ unmount }: { unmount: () => void }) => { + const { data, loading } = useQuery(CAR_QUERY, { pollInterval: 10 }); + switch (++renderCount) { + case 1: + case 2: + expect(loading).toBeTruthy(); + break; + case 3: + case 4: + expect(loading).toBeFalsy(); + expect(data).toEqual(CAR_RESULT_DATA); + expect(linkRequestSpy).toHaveBeenCalledTimes(1); + if (renderCount === 3) { + unmount(); + } + break; + default: + reject("unreached"); + } + return null; + }; + + const Component = () => { + const [queryMounted, setQueryMounted] = useState(true); + const unmount = () => setTimeout(() => setQueryMounted(false), 0); + return <>{queryMounted && }; + }; + + render( + + + + + + ); + + return wait(() => { + expect(linkRequestSpy).toHaveBeenCalledTimes(1); + }).then(resolve, reject); + }); + itAsync( 'should not throw an error if `stopPolling` is called manually after ' + 'a component has unmounted (even though polling has already been ' + From 83278295d876c92a311a401bdcf38d2efc673aa8 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 22 Jun 2021 17:38:22 -0400 Subject: [PATCH 18/19] rename getQueryResult to getExecuteResult --- src/react/data/QueryData.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts index 22a8c3af732..9d97f7283e5 100644 --- a/src/react/data/QueryData.ts +++ b/src/react/data/QueryData.ts @@ -155,10 +155,6 @@ export class QueryData extends OperationData< this.onNewData(); }; - private getExecuteResult(): QueryResult { - return this.getQueryResult(); - }; - private getExecuteSsrResult() { const { ssr, skip } = this.getOptions(); const ssrDisabled = ssr === false; @@ -182,7 +178,7 @@ export class QueryData extends OperationData< } if (this.ssrInitiated()) { - const result = this.getQueryResult() || ssrLoading; + const result = this.getExecuteResult() || ssrLoading; if (result.loading && !skip) { this.context.renderPromises!.addQueryPromise(this, () => null); } @@ -339,7 +335,7 @@ export class QueryData extends OperationData< } } - private getQueryResult = (): QueryResult => { + private getExecuteResult(): QueryResult { let result = this.observableQueryFields() as QueryResult; const options = this.getOptions(); From 5d1db1501b7bc843a62750e98e20de09c079917e Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 23 Jun 2021 11:14:59 -0400 Subject: [PATCH 19/19] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e64edb79fab..74df99fd6e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,9 @@ - `InMemoryCache` now coalesces `EntityStore` updates to guarantee only one `store.merge(id, fields)` call per `id` per cache write.
[@benjamn](https://github.com/benjamn) in [#8372](https://github.com/apollographql/apollo-client/pull/8372) +- Fix polling when used with `React.StrictMode`,
+ [@brainkim](https://github.com/brainkim) in [#8414](https://github.com/apollographql/apollo-client/pull/8414) + ### Potentially disruptive changes - To avoid retaining sensitive information from mutation root field arguments, Apollo Client v3.4 automatically clears any `ROOT_MUTATION` fields from the cache after each mutation finishes. If you need this information to remain in the cache, you can prevent the removal by passing the `keepRootFields: true` option to `client.mutate`. `ROOT_MUTATION` result data are also passed to the mutation `update` function, so we recommend obtaining the results that way, rather than using `keepRootFields: true`, if possible.