resolve comments rd.2

Author: Brian Chen

.changeset/rich-birds-wink.md

@@ -1,4 +1,4 @@
 ---
 '@firebase/firestore': minor
 ---
-A write to a document that contains field transforms is no longer split into separate operations. This reduces the number of writes that the backend performs.
+A write to a document that contains FieldValue transforms is no longer split up into two separate operations. This reduces the number of writes the backend performs and allows each WriteBatch to hold 500 writes regardless of how many FieldValue transformations are attached.

packages/firestore/src/api/user_data_reader.ts

@@ -102,6 +102,7 @@ export class ParsedSetData {
 export class ParsedUpdateData {
   constructor(
     readonly data: ObjectValue,
+    // The fieldMask does not include document transforms.
     readonly fieldMask: FieldMask,
     readonly fieldTransforms: FieldTransform[]
   ) {}

packages/firestore/src/model/mutation.ts

@@ -428,20 +428,20 @@ export function mutationEquals(left: Mutation, right: Mutation): boolean {
     return false;
   }
 
+  if (!fieldTransformsAreEqual(left.fieldTransforms, right.fieldTransforms)) {
+    return false;
+  }
+
   if (left.type === MutationType.Set) {
-    return (
-      (left as SetMutation).value.isEqual((right as SetMutation).value) &&
-      fieldTransformsAreEqual(left.fieldTransforms, right.fieldTransforms)
-    );
+    return (left as SetMutation).value.isEqual((right as SetMutation).value);
   }
 
   if (left.type === MutationType.Patch) {
     return (
       (left as PatchMutation).data.isEqual((right as PatchMutation).data) &&
       (left as PatchMutation).fieldMask.isEqual(
         (right as PatchMutation).fieldMask
-      ) &&
-      fieldTransformsAreEqual(left.fieldTransforms, right.fieldTransforms)
+      )
     );
   }
 
@@ -502,7 +502,7 @@ function applySetMutationToRemoteDocument(
   // remote document the server has accepted the mutation so the precondition
   // must have held.
   let newData = mutation.value;
-  if (mutation.fieldTransforms && mutationResult.transformResults) {
+  if (mutationResult.transformResults) {
     const transformResults = serverTransformResults(
       mutation.fieldTransforms,
       maybeDoc,
@@ -635,7 +635,7 @@ function applyPatchMutationToLocalView(
 function patchDocument(
   mutation: PatchMutation,
   maybeDoc: MaybeDocument | null,
-  transformResults?: ProtoValue[]
+  transformResults: ProtoValue[]
 ): ObjectValue {
   let data: ObjectValue;
   if (maybeDoc instanceof Document) {
@@ -644,9 +644,7 @@ function patchDocument(
     data = ObjectValue.empty();
   }
   data = patchObject(mutation, data);
-  if (transformResults) {
-    data = transformObject(mutation.fieldTransforms, data, transformResults);
-  }
+  data = transformObject(mutation.fieldTransforms, data, transformResults);
   return data;
 }
 

packages/firestore/src/remote/serializer.ts

@@ -634,7 +634,7 @@ export function toMutation(
     return fail('Unknown mutation type ' + mutation.type);
   }
 
-  if (mutation.fieldTransforms && mutation.fieldTransforms.length > 0) {
+  if (mutation.fieldTransforms.length > 0) {
     result.updateTransforms = mutation.fieldTransforms.map(transform =>
       toFieldTransform(serializer, transform)
     );
@@ -659,7 +659,7 @@ export function fromMutation(
     ? proto.updateTransforms.map(transform =>
         fromFieldTransform(serializer, transform)
       )
-    : undefined;
+    : [];
 
   if (proto.update) {
     assertPresent(proto.update.name, 'name');

packages/firestore/test/unit/local/local_serializer.test.ts

@@ -69,7 +69,7 @@ describe('Local Serializer', () => {
     currentDocument: { exists: true }
   };
   // Updated transform proto representation.
-  const updateTransformWrite = {
+  const updateTransforms = {
     updateTransforms: [
       { fieldPath: 'integer', increment: { integerValue: '42' } },
       { fieldPath: 'double', increment: { doubleValue: 13.37 } }
@@ -90,7 +90,7 @@ describe('Local Serializer', () => {
     const serialized = toMutation(s, mutationBatch.mutations[0]);
     expect(serialized).to.deep.equal({
       ...setMutationWrite,
-      ...updateTransformWrite
+      ...updateTransforms
     });
   });
 
@@ -108,7 +108,7 @@ describe('Local Serializer', () => {
     const serialized = toMutation(s, mutationBatch.mutations[0]);
     expect(serialized).to.deep.equal({
       ...patchMutationWrite,
-      ...updateTransformWrite
+      ...updateTransforms
     });
   });
 
@@ -180,9 +180,9 @@ describe('Local Serializer', () => {
     );
     const expected = [
       setMutationWrite,
-      { ...setMutationWrite, ...updateTransformWrite },
+      { ...setMutationWrite, ...updateTransforms },
       deleteMutationWrite,
-      { ...patchMutationWrite, ...updateTransformWrite },
+      { ...patchMutationWrite, ...updateTransforms },
       patchMutationWrite
     ];
     const mutationBatch = fromDbMutationBatch(localSerializer, dbMutationBatch);

packages/firestore/test/unit/model/mutation.test.ts

@@ -247,16 +247,15 @@ describe('Mutation', () => {
       foo: FieldValue.arrayUnion('tag'),
       'bar.baz': FieldValue.arrayUnion(true, { nested: { a: [1, 2] } })
     });
-    expect(transform.fieldTransforms).to.not.be.undefined;
     expect(transform.fieldTransforms).to.have.lengthOf(2);
 
-    const first = transform.fieldTransforms![0];
+    const first = transform.fieldTransforms[0];
     expect(first.field).to.deep.equal(field('foo'));
     expect(first.transform).to.deep.equal(
       new ArrayUnionTransformOperation([wrap('tag')])
     );
 
-    const second = transform.fieldTransforms![1];
+    const second = transform.fieldTransforms[1];
     expect(second.field).to.deep.equal(field('bar.baz'));
     expect(second.transform).to.deep.equal(
       new ArrayUnionTransformOperation([
@@ -273,10 +272,9 @@ describe('Mutation', () => {
     const transform = patchMutation('collection/key', {
       foo: FieldValue.arrayRemove('tag')
     });
-    expect(transform.fieldTransforms).to.not.be.undefined;
     expect(transform.fieldTransforms).to.have.lengthOf(1);
 
-    const first = transform.fieldTransforms![0];
+    const first = transform.fieldTransforms[0];
     expect(first.field).to.deep.equal(field('foo'));
     expect(first.transform).to.deep.equal(
       new ArrayRemoveTransformOperation([wrap('tag')])