Review feedback

Author: wilhuff

packages/firestore/src/api/user_data_reader.ts

@@ -738,14 +738,15 @@ export function parseData(
     // context.fieldMask and we return null as our parsing result.
     parseSentinelFieldValue(input, context);
     return null;
+  } else if (input === undefined && context.ignoreUndefinedProperties) {
+    // If the input is undefined it can never participate in the fieldMask, so
+    // don't handle this below. If `ignoreUndefinedProperties` is false,
+    // `parseScalarValue` will reject an undefined value.
+    return null;
   } else {
     // If context.path is null we are inside an array and we don't support
     // field mask paths more granular than the top-level array.
-    //
-    // If the input is undefined it can never participate in the fieldMask. With
-    // `ignoreUndefinedProperties` set to false, `parseScalarValue` will reject
-    // an undefined value.
-    if (context.path && input !== undefined) {
+    if (context.path) {
       context.fieldMask.push(context.path);
     }
 
@@ -900,8 +901,6 @@ function parseScalarValue(
         value._key.path
       )
     };
-  } else if (value === undefined && context.ignoreUndefinedProperties) {
-    return null;
   } else {
     throw context.createError(
       `Unsupported field value: ${valueDescription(value)}`

packages/firestore/test/integration/api/fields.test.ts

@@ -400,8 +400,8 @@ apiDescribe('`undefined` properties', (persistence: boolean) => {
 
   it('are ignored in set({ merge: true })', () => {
     return withTestDocAndSettings(persistence, settings, async doc => {
-      await doc.set({ foo: 'foo', 'bar': 'unchanged' });
-      await doc.set({ foo: 'foo', 'bar': undefined }, { merge: true });
+      await doc.set({ foo: 'foo', bar: 'unchanged' });
+      await doc.set({ foo: 'foo', bar: undefined }, { merge: true });
       const docSnap = await doc.get();
       expect(docSnap.data()).to.deep.equal({ foo: 'foo', bar: 'unchanged' });
     });