API tweaks from review. Selectable, FilterCondition, and Accumulator are abstract classes not interfaces. Removed extraneous Pipeline.sort overload

MarkDuckworth · MarkDuckworth · commit 50a65e43c431 · 2025-01-23T12:51:04.000-07:00
diff --git a/packages/firestore/src/core/pipeline-util.ts b/packages/firestore/src/core/pipeline-util.ts
@@ -29,24 +29,13 @@ import {
   gt,
   lte,
   gte,
-  Expr,
   eq,
   Or
 } from '../lite-api/expressions';
 import { Pipeline } from '../lite-api/pipeline';
 import { doc } from '../lite-api/reference';
 import { isNanValue, isNullValue } from '../model/values';
-import {
-  ArrayValue as ProtoArrayValue,
-  Function as ProtoFunction,
-  LatLng as ProtoLatLng,
-  MapValue as ProtoMapValue,
-  Pipeline as ProtoPipeline,
-  Timestamp as ProtoTimestamp,
-  Value as ProtoValue
-} from '../protos/firestore_proto_api';
 import { fail } from '../util/assert';
-import { isPlainObject } from '../util/input_validation';
 
 import { Bound } from './bound';
 import {
@@ -67,131 +56,6 @@ import {
 
 /* eslint @typescript-eslint/no-explicit-any: 0 */
 
-function isITimestamp(obj: any): obj is ProtoTimestamp {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-  if (
-    'seconds' in obj &&
-    (obj.seconds === null ||
-      typeof obj.seconds === 'number' ||
-      typeof obj.seconds === 'string') &&
-    'nanos' in obj &&
-    (obj.nanos === null || typeof obj.nanos === 'number')
-  ) {
-    return true;
-  }
-
-  return false;
-}
-function isILatLng(obj: any): obj is ProtoLatLng {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-  if (
-    'latitude' in obj &&
-    (obj.latitude === null || typeof obj.latitude === 'number') &&
-    'longitude' in obj &&
-    (obj.longitude === null || typeof obj.longitude === 'number')
-  ) {
-    return true;
-  }
-
-  return false;
-}
-function isIArrayValue(obj: any): obj is ProtoArrayValue {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-  if ('values' in obj && (obj.values === null || Array.isArray(obj.values))) {
-    return true;
-  }
-
-  return false;
-}
-function isIMapValue(obj: any): obj is ProtoMapValue {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-  if ('fields' in obj && (obj.fields === null || isPlainObject(obj.fields))) {
-    return true;
-  }
-
-  return false;
-}
-function isIFunction(obj: any): obj is ProtoFunction {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-  if (
-    'name' in obj &&
-    (obj.name === null || typeof obj.name === 'string') &&
-    'args' in obj &&
-    (obj.args === null || Array.isArray(obj.args))
-  ) {
-    return true;
-  }
-
-  return false;
-}
-
-function isIPipeline(obj: any): obj is ProtoPipeline {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-  if ('stages' in obj && (obj.stages === null || Array.isArray(obj.stages))) {
-    return true;
-  }
-
-  return false;
-}
-
-export function isFirestoreValue(obj: any): obj is ProtoValue {
-  if (typeof obj !== 'object' || obj === null) {
-    return false; // Must be a non-null object
-  }
-
-  // Check optional properties and their types
-  if (
-    ('nullValue' in obj &&
-      (obj.nullValue === null || obj.nullValue === 'NULL_VALUE')) ||
-    ('booleanValue' in obj &&
-      (obj.booleanValue === null || typeof obj.booleanValue === 'boolean')) ||
-    ('integerValue' in obj &&
-      (obj.integerValue === null ||
-        typeof obj.integerValue === 'number' ||
-        typeof obj.integerValue === 'string')) ||
-    ('doubleValue' in obj &&
-      (obj.doubleValue === null || typeof obj.doubleValue === 'number')) ||
-    ('timestampValue' in obj &&
-      (obj.timestampValue === null || isITimestamp(obj.timestampValue))) ||
-    ('stringValue' in obj &&
-      (obj.stringValue === null || typeof obj.stringValue === 'string')) ||
-    ('bytesValue' in obj &&
-      (obj.bytesValue === null || obj.bytesValue instanceof Uint8Array)) ||
-    ('referenceValue' in obj &&
-      (obj.referenceValue === null ||
-        typeof obj.referenceValue === 'string')) ||
-    ('geoPointValue' in obj &&
-      (obj.geoPointValue === null || isILatLng(obj.geoPointValue))) ||
-    ('arrayValue' in obj &&
-      (obj.arrayValue === null || isIArrayValue(obj.arrayValue))) ||
-    ('mapValue' in obj &&
-      (obj.mapValue === null || isIMapValue(obj.mapValue))) ||
-    ('fieldReferenceValue' in obj &&
-      (obj.fieldReferenceValue === null ||
-        typeof obj.fieldReferenceValue === 'string')) ||
-    ('functionValue' in obj &&
-      (obj.functionValue === null || isIFunction(obj.functionValue))) ||
-    ('pipelineValue' in obj &&
-      (obj.pipelineValue === null || isIPipeline(obj.pipelineValue)))
-  ) {
-    return true;
-  }
-
-  return false;
-}
-
 export function toPipelineFilterCondition(f: FilterInternal): FilterCondition {
   if (f instanceof FieldFilterInternal) {
     const field = Field.of(f.field.toString());
@@ -355,7 +219,7 @@ function whereConditionsFromCursor(
   bound: Bound,
   orderings: Ordering[],
   position: 'before' | 'after'
-): Expr {
+): FilterCondition {
   const cursors = bound.position.map(value => Constant._fromProto(value));
   const filterFunc = position === 'before' ? lt : gt;
   const filterInclusiveFunc = position === 'before' ? lte : gte;
diff --git a/packages/firestore/src/lite-api/expressions.ts b/packages/firestore/src/lite-api/expressions.ts
@@ -17,7 +17,6 @@
 
 /* eslint @typescript-eslint/no-explicit-any: 0 */
 
-import { isFirestoreValue } from '../core/pipeline-util';
 import {
   DOCUMENT_KEY_NAME,
   FieldPath as InternalFieldPath
@@ -30,6 +29,7 @@ import {
   UserData
 } from '../remote/serializer';
 import { hardAssert } from '../util/assert';
+import { isFirestoreValue } from '../util/proto';
 
 import { documentId, FieldPath } from './field_path';
 import { GeoPoint } from './geo_point';
@@ -44,45 +44,6 @@ import {
 } from './user_data_reader';
 import { VectorValue } from './vector_value';
 
-/**
- * @beta
- *
- * An interface that represents a selectable expression.
- */
-export interface Selectable extends Expr {
-  selectable: true;
-}
-
-/**
- * @beta
- *
- * An interface that represents a filter condition.
- */
-export interface FilterCondition extends Expr {
-  filterable: true;
-}
-
-/**
- * @beta
- *
- * An interface that represents an accumulator.
- */
-export interface Accumulator extends Expr {
-  accumulator: true;
-  /**
-   * @private
-   * @internal
-   */
-  _toProto(serializer: JsonProtoSerializer): ProtoValue;
-}
-
-/**
- * @beta
- *
- * An accumulator target, which is an expression with an alias that also implements the Accumulator interface.
- */
-export type AccumulatorTarget = ExprWithAlias<Accumulator>;
-
 /**
  * @beta
  *
@@ -1821,10 +1782,50 @@ export abstract class Expr implements ProtoSerializable<ProtoValue>, UserData {
   abstract _readUserData(dataReader: UserDataReader): void;
 }
 
+/**
+ * @beta
+ *
+ * An interface that represents a selectable expression.
+ */
+export abstract class Selectable extends Expr {
+  selectable: true = true;
+}
+
+/**
+ * @beta
+ *
+ * An interface that represents a filter condition.
+ */
+export abstract class FilterCondition extends Expr {
+  filterable: true = true;
+}
+
+/**
+ * @beta
+ *
+ * An interface that represents an accumulator.
+ */
+export abstract class Accumulator extends Expr {
+  accumulator: true = true;
+
+  /**
+   * @private
+   * @internal
+   */
+  abstract _toProto(serializer: JsonProtoSerializer): ProtoValue;
+}
+
+/**
+ * @beta
+ *
+ * An accumulator target, which is an expression with an alias that also implements the Accumulator interface.
+ */
+export type AccumulatorTarget = ExprWithAlias<Accumulator>;
+
 /**
  * @beta
  */
-export class ExprWithAlias<T extends Expr> extends Expr implements Selectable {
+export class ExprWithAlias<T extends Expr> extends Selectable {
   exprType: ExprType = 'ExprWithAlias';
   selectable = true as const;
 
@@ -1897,7 +1898,7 @@ class ListOfExprs extends Expr {
  * const cityField = Field.of("address.city");
  * ```
  */
-export class Field extends Expr implements Selectable {
+export class Field extends Selectable {
   exprType: ExprType = 'Field';
   selectable = true as const;
 
@@ -1927,7 +1928,6 @@ export class Field extends Expr implements Selectable {
    */
   static of(name: string): Field;
   static of(path: FieldPath): Field;
-  static of(pipeline: Pipeline, name: string): Field;
   static of(
     pipelineOrName: Pipeline | string | FieldPath,
     name?: string
@@ -1974,7 +1974,7 @@ export class Field extends Expr implements Selectable {
 /**
  * @beta
  */
-export class Fields extends Expr implements Selectable {
+export class Fields extends Selectable {
   exprType: ExprType = 'Field';
   selectable = true as const;
 
diff --git a/packages/firestore/src/lite-api/pipeline.ts b/packages/firestore/src/lite-api/pipeline.ts
@@ -620,7 +620,6 @@ export class Pipeline implements ProtoSerializable<ExecutePipelineRequest> {
    * @return A new {@code Pipeline} object with this stage appended to the stage list.
    */
   sort(...orderings: Ordering[]): Pipeline;
-  sort(options: { orderings: Ordering[] }): Pipeline;
   sort(
     optionsOrOrderings:
       | Ordering
diff --git a/packages/firestore/src/util/proto.ts b/packages/firestore/src/util/proto.ts
