diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ed3fad9382..5d5d419d16f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ - Fix error thrown by nested `keyFields: ["a", ["b", "c"], "d"]` type policies when writing results into the cache where any of the key fields (`.a`, `.a.b`, `.a.c`, or `.d`) have been renamed by query field alias syntax.
[@benjamn](https://github.com/benjamn) in [#8643](https://github.com/apollographql/apollo-client/pull/8643) +- Fix regression from PR [#8422](https://github.com/apollographql/apollo-client/pull/8422) (first released in `@apollo/client@3.4.0-rc.15`) that caused `result.data` to be set to undefined in some cases after `ObservableQuery#getCurrentResult` reads an incomplete result from the cache.
+ [@benjamn](https://github.com/benjamn) in [#8642](https://github.com/apollographql/apollo-client/pull/8642) + ## Apollo Client 3.4.7 ### Bug Fixes diff --git a/package.json b/package.json index b109cdf0258..5c33c2f9d9e 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,7 @@ { "name": "apollo-client", "path": "./dist/apollo-core.cjs.min.js", - "maxSize": "24.6 kB" + "maxSize": "24.7 kB" } ], "engines": { diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index 1aad0f8b987..1e73b8225e9 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -2590,15 +2590,16 @@ describe('client', () => { subscription.unsubscribe(); const lastError = observable.getLastError(); - const lastResult = observable.getLastResult(); + expect(lastError).toBeInstanceOf(ApolloError); + expect(lastError!.networkError).toEqual(error); + const lastResult = observable.getLastResult(); expect(lastResult).toBeTruthy(); expect(lastResult!.loading).toBe(false); expect(lastResult!.networkStatus).toBe(8); observable.resetLastResults(); subscription = observable.subscribe(observerOptions); - Object.assign(observable, { lastError, lastResult }); // The error arrived, run a refetch to get the third result // which should now contain valid data. diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 3b339d84401..6038a87755f 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -45,6 +45,12 @@ export interface UpdateQueryOptions { let warnedAboutUpdateQuery = false; +interface Last { + result: ApolloQueryResult; + variables?: TVariables; + error?: ApolloError; +} + export class ObservableQuery< TData = any, TVariables = OperationVariables @@ -68,12 +74,15 @@ export class ObservableQuery< private observers = new Set>>(); private subscriptions = new Set(); - private lastResult: ApolloQueryResult | undefined; - private lastResultSnapshot: ApolloQueryResult | undefined; - private lastError: ApolloError | undefined; + private last?: Last; + private queryInfo: QueryInfo; + // When this.concast is defined, this.observer is the Observer currently + // subscribed to that Concast. private concast?: Concast>; + private observer?: Observer>; + private pollingInfo?: { interval: number; timeout: ReturnType; @@ -102,10 +111,11 @@ export class ObservableQuery< 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); + const last = this.last; + if (last && last.error) { + observer.error && observer.error(last.error); + } else if (last && last.result) { + observer.next && observer.next(last.result); } // Initiate observation of this query if it hasn't been reported to @@ -177,12 +187,8 @@ export class ObservableQuery< } public getCurrentResult(saveAsLastResult = true): ApolloQueryResult { - const { - lastResult, - options: { - fetchPolicy = "cache-first", - }, - } = this; + // Use the last result as long as the variables match this.variables. + const lastResult = this.getLastResult(true); const networkStatus = this.queryInfo.networkStatus || @@ -202,11 +208,14 @@ export class ObservableQuery< if (!this.queryManager.transform(this.options.query).hasForcedResolvers) { const diff = this.queryInfo.getDiff(); - result.data = ( - diff.complete || - this.options.returnPartialData - ) ? diff.result : void 0; + if (diff.complete || this.options.returnPartialData) { + result.data = diff.result; + } + if (equal(result.data, {})) { + result.data = void 0 as any; + } + const { fetchPolicy = "cache-first" } = this.options; if (diff.complete) { // If the diff is complete, and we're using a FetchPolicy that // terminates after a complete cache read, we can assume the next @@ -247,23 +256,31 @@ export class ObservableQuery< // Compares newResult to the snapshot we took of this.lastResult when it was // first received. public isDifferentFromLastResult(newResult: ApolloQueryResult) { - return !equal(this.lastResultSnapshot, newResult); + return !this.last || !equal(this.last.result, newResult); + } + + private getLast>( + key: K, + variablesMustMatch?: boolean, + ) { + const last = this.last; + if (last && + last[key] && + (!variablesMustMatch || equal(last!.variables, this.variables))) { + return last[key]; + } } - // Returns the last result that observer.next was called with. This is not the same as - // getCurrentResult! If you're not sure which you need, then you probably need getCurrentResult. - public getLastResult(): ApolloQueryResult | undefined { - return this.lastResult; + public getLastResult(variablesMustMatch?: boolean): ApolloQueryResult | undefined { + return this.getLast("result", variablesMustMatch); } - public getLastError(): ApolloError | undefined { - return this.lastError; + public getLastError(variablesMustMatch?: boolean): ApolloError | undefined { + return this.getLast("error", variablesMustMatch); } public resetLastResults(): void { - delete this.lastResult; - delete this.lastResultSnapshot; - delete this.lastError; + delete this.last; this.isTornDown = false; } @@ -499,7 +516,6 @@ once, rather than every time you call fetchMore.`); const { result } = queryManager.cache.diff({ query: this.options.query, variables: this.variables, - previousResult: this.lastResult?.data, returnPartialData: true, optimistic: false, }); @@ -599,16 +615,21 @@ once, rather than every time you call fetchMore.`); poll(); } - private updateLastResult(newResult: ApolloQueryResult) { - const previousResult = this.lastResult; - this.lastResult = newResult; - this.lastResultSnapshot = this.queryManager.assumeImmutableResults - ? newResult - : cloneDeep(newResult); + private updateLastResult( + newResult: ApolloQueryResult, + variables = this.variables, + ) { + this.last = { + ...this.last, + result: this.queryManager.assumeImmutableResults + ? newResult + : cloneDeep(newResult), + variables, + }; if (!isNonEmptyArray(newResult.errors)) { - delete this.lastError; + delete this.last.error; } - return previousResult; + return this.last; } public reobserve( @@ -657,7 +678,16 @@ once, rather than every time you call fetchMore.`); } } + const variables = options.variables && { ...options.variables }; const concast = this.fetch(options, newNetworkStatus); + const observer: Observer> = { + next: result => { + this.reportResult(result, variables); + }, + error: error => { + this.reportError(error, variables); + }, + }; if (!useDisposableConcast) { // We use the {add,remove}Observer methods directly to avoid wrapping @@ -665,14 +695,15 @@ once, rather than every time you call fetchMore.`); // 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. - if (this.concast) { + if (this.concast && this.observer) { this.concast.removeObserver(this.observer, true); } this.concast = concast; + this.observer = observer; } - concast.addObserver(this.observer); + concast.addObserver(observer); return concast.promise; } @@ -684,31 +715,40 @@ once, rather than every time you call fetchMore.`); // save the fetchMore result as this.lastResult, causing it to be // ignored due to the this.isDifferentFromLastResult check in // this.observer.next. - this.observer.next(this.getCurrentResult(false)); + this.reportResult( + this.getCurrentResult(false), + this.variables, + ); } - private observer = { - next: (result: ApolloQueryResult) => { - if (this.lastError || this.isDifferentFromLastResult(result)) { - this.updateLastResult(result); - iterateObserversSafely(this.observers, 'next', result); - } - }, + private reportResult( + result: ApolloQueryResult, + variables: TVariables | undefined, + ) { + if (this.getLastError() || this.isDifferentFromLastResult(result)) { + this.updateLastResult(result, variables); + iterateObserversSafely(this.observers, 'next', result); + } + } - error: (error: ApolloError) => { - // Since we don't get the current result on errors, only the error, we - // must mirror the updates that occur in QueryStore.markQueryError here - this.updateLastResult({ - ...this.lastResult, - error, - errors: error.graphQLErrors, - networkStatus: NetworkStatus.error, - loading: false, - } as ApolloQueryResult); - - iterateObserversSafely(this.observers, 'error', this.lastError = error); - }, - }; + private reportError( + error: ApolloError, + variables: TVariables | undefined, + ) { + // Since we don't get the current result on errors, only the error, we + // must mirror the updates that occur in QueryStore.markQueryError here + const errorResult = { + ...this.getLastResult(), + error, + errors: error.graphQLErrors, + networkStatus: NetworkStatus.error, + loading: false, + } as ApolloQueryResult; + + this.updateLastResult(errorResult, variables); + + iterateObserversSafely(this.observers, 'error', this.last!.error = error); + } public hasObservers() { return this.observers.size > 0; @@ -716,9 +756,10 @@ once, rather than every time you call fetchMore.`); private tearDownQuery() { if (this.isTornDown) return; - if (this.concast) { + if (this.concast && this.observer) { this.concast.removeObserver(this.observer); delete this.concast; + delete this.observer; } this.stopPolling(); diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index d3df4d89abe..94bea64470a 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -798,7 +798,7 @@ describe('ObservableQuery', () => { expect(result.errors).toEqual([error]); expect(observable.getCurrentResult().errors).toEqual([error]); observable.setVariables(differentVariables); - expect(observable.getCurrentResult().errors).toEqual([error]); + expect(observable.getCurrentResult().errors).toBeUndefined(); } else if (handleCount === 2) { expect(result.data).toEqual(dataTwo); expect(observable.getCurrentResult().data).toEqual( diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts index 7975af5fbd4..71e05d0f718 100644 --- a/src/react/data/QueryData.ts +++ b/src/react/data/QueryData.ts @@ -323,14 +323,13 @@ export class QueryData extends OperationData< // has a chance to stay open). const { currentObservable } = this; if (currentObservable) { - const lastError = currentObservable.getLastError(); - const lastResult = currentObservable.getLastResult(); - currentObservable.resetLastResults(); - this.startQuerySubscription(); - Object.assign(currentObservable, { - lastError, - lastResult - }); + const last = currentObservable["last"]; + try { + currentObservable.resetLastResults(); + this.startQuerySubscription(); + } finally { + currentObservable["last"] = last; + } } } diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 61a8057c853..967f4ab74c9 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -18,7 +18,6 @@ import { ApolloLink } from '../../../link/core'; import { itAsync, MockLink, MockedProvider, mockSingleLink } from '../../../testing'; import { useQuery } from '../useQuery'; import { useMutation } from '../useMutation'; -import { invariant } from 'ts-invariant'; describe('useQuery Hook', () => { describe('General use', () => { @@ -2438,7 +2437,6 @@ describe('useQuery Hook', () => { describe('Missing Fields', () => { it('should log debug messages about MissingFieldErrors from the cache', async () => { const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - const debugSpy = jest.spyOn(invariant, 'debug').mockImplementation(() => {}); const carQuery: DocumentNode = gql` query cars($id: Int) { @@ -2488,15 +2486,8 @@ describe('useQuery Hook', () => { expect(result.current.error).toBe(undefined); await waitForNextUpdate(); expect(result.current.loading).toBe(false); - expect(result.current.data).toBe(undefined); - + expect(result.current.data).toEqual(carData); expect(result.current.error).toBeUndefined(); - expect(debugSpy).toHaveBeenCalledTimes(1); - expect(debugSpy).toHaveBeenLastCalledWith( - "Missing cache result fields: cars.0.vin", - [new Error("Can't find field 'vin' on Car:1 object")] - ); - debugSpy.mockRestore(); expect(errorSpy).toHaveBeenCalledTimes(1); expect(errorSpy).toHaveBeenLastCalledWith(