fix: make SetOptions a union type (#1618)

googleapis · Oct 14, 2021 · 1d8bfd6 · 1d8bfd6
1 parent bfa3293
commit 1d8bfd6
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 76 deletions.
diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts
@@ -315,8 +315,8 @@ export class WriteBatch implements firestore.WriteBatch {
     options?: firestore.SetOptions
   ): WriteBatch {
     validateSetOptions('options', options, {optional: true});
-    const mergeLeaves = options && options.merge === true;
-    const mergePaths = options && options.mergeFields;
+    const mergeLeaves = options && 'merge' in options && options.merge;
+    const mergePaths = options && 'mergeFields' in options;
     const ref = validateDocumentReference('documentRef', documentRef);
     let firestoreData: firestore.DocumentData;
     if (mergeLeaves || mergePaths) {
@@ -763,27 +763,9 @@ export function validateSetOptions(
       );
     }
 
-    const setOptions = value as {[k: string]: unknown};
-
-    if ('merge' in setOptions && typeof setOptions.merge !== 'boolean') {
-      throw new Error(
-        `${invalidArgumentMessage(
-          arg,
-          'set() options argument'
-        )} "merge" is not a boolean.`
-      );
-    }
+    const setOptions = value as {mergeFields: Array<string | FieldPath>};
 
     if ('mergeFields' in setOptions) {
-      if (!Array.isArray(setOptions.mergeFields)) {
-        throw new Error(
-          `${invalidArgumentMessage(
-            arg,
-            'set() options argument'
-          )} "mergeFields" is not an array.`
-        );
-      }
-
       for (let i = 0; i < setOptions.mergeFields.length; ++i) {
         try {
           validateFieldPath(i, setOptions.mergeFields[i]);

diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts
@@ -3113,7 +3113,7 @@ describe('Types test', () => {
         testObj: PartialWithFieldValue<TestObject>,
         options?: SetOptions
       ) {
-        if (options && (options.merge || options.mergeFields)) {
+        if (options) {
           expect(testObj).to.not.be.an.instanceOf(TestObject);
         } else {
           expect(testObj).to.be.an.instanceOf(TestObject);
@@ -3220,7 +3220,7 @@ describe('Types test', () => {
         testObj: PartialWithFieldValue<TestObject>,
         options?: SetOptions
       ) {
-        if (options && (options.merge || options.mergeFields)) {
+        if (options) {
           expect(testObj).to.not.be.an.instanceOf(TestObject);
         } else {
           expect(testObj).to.be.an.instanceOf(TestObject);

diff --git a/dev/test/document.ts b/dev/test/document.ts
@@ -1308,56 +1308,28 @@ describe('set document', () => {
   });
 
   it('validates merge option', () => {
-    expect(() => {
-      firestore
-        .doc('collectionId/documentId')
-        .set({foo: 'bar'}, 'foo' as InvalidApiUsage);
-    }).to.throw(
-      'Value for argument "options" is not a valid set() options argument. Input is not an object.'
-    );
-
-    expect(() => {
-      firestore.doc('collectionId/documentId').set(
-        {foo: 'bar'},
-        {
-          merge: 42 as InvalidApiUsage,
-        }
-      );
-    }).to.throw(
-      'Value for argument "options" is not a valid set() options argument. "merge" is not a boolean.'
-    );
-
     expect(() => {
       firestore.doc('collectionId/documentId').set(
         {foo: 'bar'},
         {
-          mergeFields: 42 as InvalidApiUsage,
+          mergeFields: ['foobar'],
         }
       );
-    }).to.throw(
-      'Value for argument "options" is not a valid set() options argument. "mergeFields" is not an array.'
-    );
+    }).to.throw('Input data is missing for field "foobar".');
 
     expect(() => {
       firestore.doc('collectionId/documentId').set(
         {foo: 'bar'},
         {
-          mergeFields: [null as InvalidApiUsage],
+          mergeFields: ['foobar..'],
         }
       );
     }).to.throw(
-      'Value for argument "options" is not a valid set() options argument. "mergeFields" is not valid: Element at index 0 is not a valid field path. Paths can only be specified as strings or via a FieldPath object.'
+      'Value for argument "options" is not a valid set() options argument. ' +
+        '"mergeFields" is not valid: Element at index 0 is not a valid ' +
+        'field path. Paths must not contain ".." in them.'
     );
 
-    expect(() => {
-      firestore.doc('collectionId/documentId').set(
-        {foo: 'bar'},
-        {
-          mergeFields: ['foobar'],
-        }
-      );
-    }).to.throw('Input data is missing for field "foobar".');
-
     expect(() => {
       firestore
         .doc('collectionId/documentId')

diff --git a/dev/test/util/helpers.ts b/dev/test/util/helpers.ts
@@ -354,7 +354,7 @@ export const postConverterMerge = {
     post: PartialWithFieldValue<Post>,
     options?: SetOptions
   ): DocumentData {
-    if (options && (options.merge || options.mergeFields)) {
+    if (options) {
       expect(post).to.not.be.an.instanceOf(Post);
     } else {
       expect(post).to.be.an.instanceof(Post);

diff --git a/types/firestore.d.ts b/types/firestore.d.ts
@@ -1065,25 +1065,22 @@ declare namespace FirebaseFirestore {
    * `DocumentReference`, `WriteBatch` and `Transaction`. These calls can be
    * configured to perform granular merges instead of overwriting the target
    * documents in their entirety.
+   *
+   * @param merge Changes the behavior of a set() call to only replace the
+   * values specified in its data argument. Fields omitted from the set() call
+   * remain untouched.
+   *
+   * @param mergeFields Changes the behavior of set() calls to only replace
+   * the specified field paths. Any field path that is not specified is ignored
+   * and remains untouched.
    */
-  export interface SetOptions {
-    /**
-     * Changes the behavior of a set() call to only replace the values specified
-     * in its data argument. Fields omitted from the set() call remain
-     * untouched.
-     */
-    readonly merge?: boolean;
-
-    /**
-     * Changes the behavior of set() calls to only replace the specified field
-     * paths. Any field path that is not specified is ignored and remains
-     * untouched.
-     *
-     * It is an error to pass a SetOptions object to a set() call that is
-     * missing a value for any of the fields specified here.
-     */
-    readonly mergeFields?: (string | FieldPath)[];
-  }
+  export type SetOptions =
+    | {
+        readonly merge?: boolean;
+      }
+    | {
+        readonly mergeFields?: Array<string | FieldPath>;
+      };
 
   /**
    * An options object that can be used to configure the behavior of `getAll()`