Comments and cleanup

Author: MarkDuckworth

packages/firestore/src/api/aggregate.ts

@@ -66,9 +66,26 @@ export function getCountFromServer(
 }
 
 /**
- * TODO
- * @param query
- * @param aggregateSpec
+ * Calculates the specified aggregations over the documents in the result
+ * set of the given query, without actually downloading the documents.
+ *
+ * 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
+ * would be prohibitively large to download entirely (e.g. thousands of documents).
+ *
+ * The result received from the server is presented, unaltered, without
+ * considering any local state. That is, documents in the local cache are not
+ * taken into consideration, neither are local modifications not yet
+ * synchronized with the server. Previously-downloaded results, if any, are not
+ * used: every request using this source necessarily involves a round trip to
+ * the server.
+ *
+ * @param query The query whose result set to aggregate over.
+ * @param aggregateSpec An `AggregateSpec` object that specifies the aggregates
+ * to perform over the result set. The AggregateSpec specifies aliases for each
+ * aggregate, which can be used to retrieve the aggregate result.
+ * @internal TODO (sum/avg) remove when public
  */
 export function getAggregateFromServer<T extends AggregateSpec>(
   query: Query<unknown>,
@@ -78,37 +95,41 @@ export function getAggregateFromServer<T extends AggregateSpec>(
   const client = ensureFirestoreConfigured(firestore);
 
   const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => {
+    // TODO (sum/avg) should alias validation be performed or should that be
+    // delegated to the backend?
+
     return new AggregateImpl(
       alias,
       aggregate.aggregateType,
       aggregate._internalFieldPath
     );
   });
 
+  // Run the aggregation and convert the results
   return firestoreClientRunAggregateQuery(
     client,
     query._query,
     internalAggregates
   ).then(aggregateResult =>
-    convertToAggregateQuerySnapshot(
-      firestore,
-      query,
-      aggregateSpec,
-      aggregateResult
-    )
+    convertToAggregateQuerySnapshot(firestore, query, aggregateResult)
   );
 }
 
+/**
+ * Converts the core aggregration result to an `AggregateQuerySnapshot`
+ * that can be returned to the consumer.
+ * @param query
+ * @param aggregateResult Core aggregation result
+ * @internal
+ */
 function convertToAggregateQuerySnapshot<T extends AggregateSpec>(
   firestore: Firestore,
   query: Query<unknown>,
-  ref: T,
   aggregateResult: ObjectValue
 ): AggregateQuerySnapshot<T> {
   const userDataWriter = new ExpUserDataWriter(firestore);
   const querySnapshot = new AggregateQuerySnapshot<T>(
     query,
-    firestore,
     userDataWriter,
     aggregateResult
   );

packages/firestore/src/core/aggregate.ts

@@ -19,6 +19,7 @@ import { FieldPath } from '../model/path';
 
 /**
  * Union type representing the aggregate type to be performed.
+ * @internal
  */
 export type AggregateType = 'count' | 'avg' | 'sum';
 

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

@@ -33,6 +33,7 @@ import { Firestore } from './database';
 import { FieldPath } from './field_path';
 import { Query, queryEqual } from './reference';
 import { LiteUserDataWriter } from './reference_impl';
+import { fieldPathFromArgument } from './user_data_reader';
 
 /**
  * Calculates the number of documents in the result set of the given query,
@@ -59,9 +60,19 @@ export function getCount(
 }
 
 /**
- * TODO
- * @param query
- * @param aggregates
+ * Calculates the specified aggregations over the documents in the result
+ * set of the given query, without actually downloading the documents.
+ *
+ * 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
+ * would be prohibitively large to download entirely (e.g. thousands of documents).
+ *
+ * @param query The query whose result set to aggregate over.
+ * @param aggregateSpec An `AggregateSpec` object that specifies the aggregates
+ * to perform over the result set. The AggregateSpec specifies aliases for each
+ * aggregate, which can be used to retrieve the aggregate result.
+ * @internal TODO (sum/avg) remove when public
  */
 export function getAggregate<T extends AggregateSpec>(
   query: Query<unknown>,
@@ -71,73 +82,76 @@ export function getAggregate<T extends AggregateSpec>(
   const datastore = getDatastore(firestore);
 
   const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => {
+    // TODO (sum/avg) should alias validation be performed or should that be
+    // delegated to the backend?
+
     return new AggregateImpl(
       alias,
       aggregate.aggregateType,
       aggregate._internalFieldPath
     );
   });
 
+  // Run the aggregation and convert the results
   return invokeRunAggregationQueryRpc(
     datastore,
     query._query,
     internalAggregates
   ).then(aggregateResult =>
-    convertToAggregateQuerySnapshot(
-      firestore,
-      query,
-      aggregateSpec,
-      aggregateResult
-    )
+    convertToAggregateQuerySnapshot(firestore, query, aggregateResult)
   );
 }
 
 function convertToAggregateQuerySnapshot<T extends AggregateSpec>(
   firestore: Firestore,
   query: Query<unknown>,
-  ref: T,
   aggregateResult: ObjectValue
 ): AggregateQuerySnapshot<T> {
   const userDataWriter = new LiteUserDataWriter(firestore);
   const querySnapshot = new AggregateQuerySnapshot<T>(
     query,
-    firestore,
     userDataWriter,
     aggregateResult
   );
   return querySnapshot;
 }
 
 /**
- * TODO
- * @param field
+ * Create an AggregateField object that can be used to compute the sum of
+ * a specified field over a range of documents in the result set of a query.
+ * @param field Specifies the field to sum across the result set.
+ * @internal TODO (sum/avg) remove when public
  */
 export function sum(field: string | FieldPath): AggregateField<number> {
-  return new AggregateField('sum', 'sum', field);
+  return new AggregateField('sum', fieldPathFromArgument('sum', field));
 }
 
 /**
- * TODO
- * @param field
+ * Create an AggregateField object that can be used to compute the average of
+ * a specified field over a range of documents in the result set of a query.
+ * @param field Specifies the field to average across the result set.
+ * @internal TODO (sum/avg) remove when public
  */
 export function average(
   field: string | FieldPath
 ): AggregateField<number | null> {
-  return new AggregateField('avg', 'average', field);
+  return new AggregateField('avg', fieldPathFromArgument('average', field));
 }
 
 /**
- * TODO
+ * Create an AggregateField object that can be used to compute the count of
+ * documents in the result set of a query.
+ * @internal TODO (sum/avg) remove when public
  */
 export function count(): AggregateField<number> {
-  return new AggregateField('count', 'count');
+  return new AggregateField('count');
 }
 
 /**
  * Compares two 'AggregateField` instances for equality.
  *
- * @param left
- * @param right
+ * @param left Compare this AggregateField to the `right`.
+ * @param right Compare this AggregateField to the `left`.
  */
 export function aggregateFieldEqual(
   left: AggregateField<unknown>,

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

@@ -19,16 +19,9 @@ import { AggregateType } from '../core/aggregate';
 import { ObjectValue } from '../model/object_value';
 import { FieldPath as InternalFieldPath } from '../model/path';
 
-import { average, count, sum } from './aggregate';
-import { Firestore } from './database';
-import { FieldPath } from './field_path';
 import { Query } from './reference';
-import { fieldPathFromArgument } from './user_data_reader';
 import { AbstractUserDataWriter } from './user_data_writer';
 
-/**
- * Union type representing the aggregate type to be performed.
- */
 export { AggregateType };
 
 /**
@@ -39,38 +32,28 @@ export class AggregateField<R> {
   /** A type string to uniquely identify instances of this class. */
   readonly type = 'AggregateField';
 
-  /**
-   * @internal
-   */
-  readonly _internalFieldPath: InternalFieldPath | undefined;
-
   /**
    * Create a new AggregateField<R>
-   * @param aggregateType
-   * @param field Optional
-   * @param methodName
+   * @param aggregateType Specifies the type of aggregation operation to perform.
+   * @param _internalFieldPath Optionally specifies the field that is aggregated.
+   * @internal
    */
   constructor(
-    public readonly aggregateType: AggregateType = 'count',
-    methodName: string = 'new AggregateField',
-    field?: string | FieldPath
-  ) {
-    if (field !== undefined) {
-      this._internalFieldPath = fieldPathFromArgument(methodName, field);
-    }
-  }
+    // TODO (sum/avg) make aggregateType public when the feature is supported
+    private readonly aggregateType: AggregateType = 'count',
+    private readonly _internalFieldPath?: InternalFieldPath
+  ) {}
 }
 
 /**
  * The union of all `AggregateField` types that are supported by Firestore.
  */
 export type AggregateFieldType =
-  | ReturnType<typeof count>
-  | ReturnType<typeof sum>
-  | ReturnType<typeof average>;
+  | AggregateField<number>
+  | AggregateField<number | null>;
 
 /**
- * A type whose property values are all `AggregateField` objects.
+ * Specifies a set of aggregations and their aliases.
  */
 export interface AggregateSpec {
   [field: string]: AggregateFieldType;
@@ -92,7 +75,6 @@ export class AggregateQuerySnapshot<T extends AggregateSpec> {
   /** A type string to uniquely identify instances of this class. */
   readonly type = 'AggregateQuerySnapshot';
 
-  // TODO(sum/avg) can this be removed?
   /**
    * The underlying query over which the aggregations recorded in this
    * `AggregateQuerySnapshot` were performed.
@@ -102,7 +84,6 @@ export class AggregateQuerySnapshot<T extends AggregateSpec> {
   /** @hideconstructor */
   constructor(
     query: Query<unknown>,
-    private readonly _firestore: Firestore,
     private readonly _userDataWriter: AbstractUserDataWriter,
     private readonly _data: ObjectValue
   ) {