PR feedback.

Author: MarkDuckworth

common/api-review/firestore-lite.api.md

@@ -23,7 +23,7 @@ export class AggregateField<T> {
 }
 
 // @public
-export type AggregateFieldType = AggregateField<number> | AggregateField<number | null>;
+export type AggregateFieldType = AggregateField<number | null>;
 
 // @public
 export class AggregateQuerySnapshot<T extends AggregateSpec> {

common/api-review/firestore.api.md

@@ -23,7 +23,7 @@ export class AggregateField<T> {
 }
 
 // @public
-export type AggregateFieldType = AggregateField<number> | AggregateField<number | null>;
+export type AggregateFieldType = AggregateField<number | null>;
 
 // @public
 export class AggregateQuerySnapshot<T extends AggregateSpec> {

packages/firestore/src/api/aggregate.ts

@@ -71,7 +71,7 @@ export function getCountFromServer(
  *
  * Using this function to perform aggregations is efficient because only the
  * final aggregation values, not the documents' data, is downloaded. This
- * function can even perform aggregations if the documents if the result set
+ * function can even perform aggregations of the documents if the result set
  * would be prohibitively large to download entirely (e.g. thousands of documents).
  *
  * The result received from the server is presented, unaltered, without

packages/firestore/src/lite-api/aggregate.ts

@@ -65,7 +65,7 @@ export function getCount(
  *
  * Using this function to perform aggregations is efficient because only the
  * final aggregation values, not the documents' data, is downloaded. This
- * function can even perform aggregations if the documents if the result set
+ * function can even perform aggregations of the documents if the result set
  * would be prohibitively large to download entirely (e.g. thousands of documents).
  *
  * @param query The query whose result set to aggregate over.
@@ -172,9 +172,9 @@ export function aggregateFieldEqual(
   return (
     left instanceof AggregateField &&
     right instanceof AggregateField &&
+    left._aggregateType === right._aggregateType &&
     left._internalFieldPath?.canonicalString() ===
-      right._internalFieldPath?.canonicalString() &&
-    left._aggregateType === right._aggregateType
+      right._internalFieldPath?.canonicalString()
   );
 }
 

packages/firestore/src/lite-api/aggregate_types.ts

@@ -45,12 +45,12 @@ export class AggregateField<T> {
   ) {}
 }
 
+// TODO (sum/avg) Update the definition of AggregateFieldType to be based
+// on the return type of `sum(..)`, `average(...)`, and `count()`
 /**
  * The union of all `AggregateField` types that are supported by Firestore.
  */
-export type AggregateFieldType =
-  | AggregateField<number>
-  | AggregateField<number | null>;
+export type AggregateFieldType = AggregateField<number | null>;
 
 /**
  * Specifies a set of aggregations and their aliases.

packages/firestore/src/remote/datastore.ts

@@ -263,9 +263,7 @@ export async function invokeRunAggregationQueryRpc(
   const filteredResult = response.filter(proto => !!proto.result);
 
   hardAssert(
-    filteredResult.length === 1 &&
-      filteredResult[0].result !== undefined &&
-      filteredResult[0].result.aggregateFields !== undefined,
+    filteredResult.length === 1,
     'Aggregation fields are missing from result.'
   );