Avoid clobbering default options with undefined values. (#6715)

* Avoid clobbering default options with undefined values.

Should fix #5978, using an approach inspired by
trojanowski/react-apollo-hooks#77.

* Mention PR #6715 in CHANGELOG.md.

CHANGELOG.md

@@ -16,6 +16,9 @@
 - Refactor `ObservableQuery#getCurrentResult` to reenable immediate delivery of warm cache results. As part of this refactoring, the `ApolloCurrentQueryResult` type was eliminated in favor of `ApolloQueryResult`. <br/>
   [@benjamn](https://github.com/benjamn) in [#6710](https://github.com/apollographql/apollo-client/pull/6710)
 
+- Avoid clobbering `defaultOptions` with `undefined` values. <br/>
+  [@benjamn](https://github.com/benjamn) in [#6715](https://github.com/apollographql/apollo-client/pull/6715)
+
 ## Improvements
 
 - Apollo Client will no longer modify `options.fetchPolicy` unless you pass `options.nextFetchPolicy` to request an explicit change in `FetchPolicy` after the current request. Although this is technically a breaking change, `options.nextFieldPolicy` makes it easy to restore the old behavior (by passing `cache-first`). <br/>

src/__tests__/__snapshots__/exports.ts.snap

@@ -309,6 +309,7 @@ Array [
   "canUseWeakMap",
   "checkDocument",
   "cloneDeep",
+  "compact",
   "concatPagination",
   "createFragmentMap",
   "getDefaultValues",

src/__tests__/client.ts

@@ -1708,7 +1708,12 @@ describe('client', () => {
         },
       });
 
-      checkCacheAndNetworkError(() => client.query({ query }));
+      checkCacheAndNetworkError(() => client.query({
+        query,
+        // This undefined value should be ignored in favor of
+        // defaultOptions.query.fetchPolicy.
+        fetchPolicy: void 0,
+      }));
     });
 
     itAsync('fetches from cache first, then network', (resolve, reject) => {
@@ -3149,7 +3154,12 @@ describe('@connection', () => {
         data: initialData,
       });
 
-      const obs = client.watchQuery({ query });
+      const obs = client.watchQuery({
+        query,
+        // This undefined value should be ignored in favor of
+        // defaultOptions.watchQuery.fetchPolicy.
+        fetchPolicy: void 0,
+      });
 
       subscribeAndCount(reject, obs, (handleCount, result) => {
         const resultData = stripSymbols(result.data);
@@ -3207,7 +3217,12 @@ describe('@connection', () => {
         },
       });
 
-      return client.mutate({ mutation }).then(result => {
+      return client.mutate({
+        mutation,
+        // This undefined value should be ignored in favor of
+        // defaultOptions.mutate.variables.
+        variables: void 0,
+      }).then(result => {
         expect(result.data).toEqual(data);
       }).then(resolve, reject);
     });

src/core/ApolloClient.ts

@@ -3,7 +3,7 @@ import { invariant, InvariantError } from 'ts-invariant';
 
 import { ApolloLink, FetchResult, GraphQLRequest, execute } from '../link/core';
 import { ApolloCache, DataProxy } from '../cache';
-import { Observable } from '../utilities';
+import { Observable, compact } from '../utilities';
 import { version } from '../version';
 import { HttpLink, UriFunction } from '../link/http';
 
@@ -280,10 +280,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
     options: WatchQueryOptions<TVariables>,
   ): ObservableQuery<T, TVariables> {
     if (this.defaultOptions.watchQuery) {
-      options = {
-        ...this.defaultOptions.watchQuery,
-        ...options,
-      } as WatchQueryOptions<TVariables>;
+      options = compact(this.defaultOptions.watchQuery, options);
     }
 
     // XXX Overwriting options is probably not the best way to do this long term...
@@ -311,9 +308,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
     options: QueryOptions<TVariables>,
   ): Promise<ApolloQueryResult<T>> {
     if (this.defaultOptions.query) {
-      options = { ...this.defaultOptions.query, ...options } as QueryOptions<
-        TVariables
-      >;
+      options = compact(this.defaultOptions.query, options);
     }
 
     invariant(
@@ -342,12 +337,8 @@ export class ApolloClient<TCacheShape> implements DataProxy {
     options: MutationOptions<T, TVariables>,
   ): Promise<FetchResult<T>> {
     if (this.defaultOptions.mutate) {
-      options = {
-        ...this.defaultOptions.mutate,
-        ...options,
-      } as MutationOptions<T, TVariables>;
+      options = compact(this.defaultOptions.mutate, options);
     }
-
     return this.queryManager.mutate<T>(options);
   }
 

src/core/Reobserver.ts

@@ -1,7 +1,7 @@
 import { WatchQueryOptions } from './watchQueryOptions';
 import { NetworkStatus } from './networkStatus';
 import { ApolloQueryResult } from './types';
-import { Observer, Concast } from '../utilities';
+import { Observer, Concast, compact } from '../utilities';
 import { invariant } from 'ts-invariant';
 
 // Given that QueryManager#fetchQueryObservable returns only a single
@@ -67,15 +67,8 @@ export class Reobserver<TData, TVars> {
   }
 
   public updateOptions(newOptions: Partial<WatchQueryOptions<TVars>>) {
-    Object.keys(newOptions).forEach(key => {
-      const value = (newOptions as any)[key];
-      if (value !== void 0) {
-        (this.options as any)[key] = value;
-      }
-    });
-
+    Object.assign(this.options, compact(newOptions));
     this.updatePolling();
-
     return this;
   }
 

src/react/data/MutationData.ts

@@ -11,6 +11,7 @@ import {
 import { OperationData } from './OperationData';
 import { OperationVariables } from '../../core';
 import { FetchResult } from '../../link/core';
+import { compact } from '../../utilities';
 
 type MutationResultWithoutClient<TData = any> = Omit<MutationResult<TData>, 'client'>;
 
@@ -82,10 +83,9 @@ export class MutationData<
   private mutate(
     mutationFunctionOptions: MutationFunctionOptions<TData, TVariables>
   ) {
-    return this.refreshClient().client.mutate({
-      ...(this.getOptions() || {}),
-      ...(mutationFunctionOptions || {})
-    });
+    return this.refreshClient().client.mutate(
+      compact(this.getOptions(), mutationFunctionOptions)
+    );
   }
 
   private onMutationStart() {

src/utilities/common/__tests__/compact.ts

@@ -0,0 +1,60 @@
+import { compact } from "../compact";
+
+const hasOwn = Object.prototype.hasOwnProperty;
+
+describe("compact", () => {
+  it("should produce an empty object when called without args", () => {
+    expect(compact()).toEqual({});
+  });
+
+  it("should merge objects without modifying them", () => {
+    const a = { a: 1, ay: "a" };
+    const b = { b: 2, bee: "b" };
+    const c = compact(a, b);
+
+    expect(c).toEqual({
+      ...a,
+      ...b,
+    });
+
+    expect(Object.keys(a)).toEqual(["a", "ay"]);
+    expect(Object.keys(b)).toEqual(["b", "bee"]);
+  });
+
+  it("should clean undefined values from single objects", () => {
+    const source = {
+      zero: 0,
+      undef: void 0,
+      three: 3,
+    };
+
+    const result = compact(source);
+
+    expect(result).toEqual({
+      zero: 0,
+      three: 3,
+    });
+
+    expect(Object.keys(result)).toEqual(["zero", "three"]);
+  });
+
+  it("should skip over undefined values in later objects", () => {
+    expect(compact(
+      { a: 1, b: 2 },
+      { b: void 0, c: 3 },
+      { a: 4, c: void 0 },
+    )).toEqual({
+      a: 4,
+      b: 2,
+      c: 3,
+    });
+  });
+
+  it("should not leave undefined properties in result object", () => {
+    const result = compact({ a: 1, b: void 0 }, { a: 2, c: void 0 });
+    expect(hasOwn.call(result, "a")).toBe(true);
+    expect(hasOwn.call(result, "b")).toBe(false);
+    expect(hasOwn.call(result, "c")).toBe(false);
+    expect(result).toEqual({ a: 2 });
+  });
+});

src/utilities/common/compact.ts

@@ -0,0 +1,19 @@
+import { TupleToIntersection } from './mergeDeep';
+
+export function compact<TArgs extends object[]>(
+  ...objects: TArgs
+): TupleToIntersection<TArgs> {
+  const result = Object.create(null);
+
+  objects.forEach(obj => {
+    if (!obj) return;
+    Object.keys(obj).forEach(key => {
+      const value = (obj as any)[key];
+      if (value !== void 0) {
+        result[key] = value;
+      }
+    });
+  });
+
+  return result;
+}

src/utilities/index.ts

@@ -84,3 +84,4 @@ export * from './observables/Concast';
 export * from './common/arrays';
 export * from './common/errorHandling';
 export * from './common/canUse';
+export * from './common/compact';