Addressing comments #2

Author: wu-hui

packages/firestore-types/index.d.ts

@@ -1054,6 +1054,13 @@ export class Query {
    */
   limit(limit: number): Query;
 
+  /**
+   * Creates and returns a new Query where only the last matching documents
+   * are returned as results.
+   *
+   * @param limit The maximum number of items to return.
+   * @return The created Query.
+   */
   limitToLast(limit: number): Query;
 
   /**

packages/firestore/src/api/database.ts

@@ -1554,12 +1554,6 @@ export class Query implements firestore.Query {
   limit(n: number): firestore.Query {
     validateExactNumberOfArgs('Query.limit', arguments, 1);
     validateArgType('Query.limit', 'number', 1, n);
-    return this.limitToFirst(n);
-  }
-
-  limitToFirst(n: number): firestore.Query {
-    validateExactNumberOfArgs('Query.limitToFirst', arguments, 1);
-    validateArgType('Query.limitToFirst', 'number', 1, n);
     this.verifyPositiveLimit(n);
     return new Query(this._query.withLimitToFirst(n), this.firestore);
   }
@@ -1857,14 +1851,14 @@ export class Query implements firestore.Query {
         complete: args[currArg + 2] as CompleteFn
       };
     }
+    this.validateHasExplicitOrderByForLimitToLast(this._query);
     return this.onSnapshotInternal(options, observer);
   }
 
   private onSnapshotInternal(
     options: ListenOptions,
     observer: PartialObserver<firestore.QuerySnapshot>
   ): Unsubscribe {
-    this.validateHasExplicitOrderByForLimitToLast(this._query);
     let errHandler = (err: Error): void => {
       console.error('Uncaught Error in onSnapshot:', err);
     };
@@ -1897,7 +1891,7 @@ export class Query implements firestore.Query {
     if (query.hasLimitToLast() && query.explicitOrderBy.length === 0) {
       throw new FirestoreError(
         Code.UNIMPLEMENTED,
-        'limitToLast() Query without explicit orderBy() is not supported yet.'
+        'limitToLast() queries require specifying an orderBy() on at least on document field.'
       );
     }
   }
@@ -1907,6 +1901,7 @@ export class Query implements firestore.Query {
     validateGetOptions('Query.get', options);
     return new Promise(
       (resolve: Resolver<firestore.QuerySnapshot>, reject: Rejecter) => {
+        this.validateHasExplicitOrderByForLimitToLast(this._query);
         if (options && options.source === 'cache') {
           this.firestore
             .ensureClientConfigured()

packages/firestore/src/core/query.ts

@@ -168,7 +168,7 @@ export class Query {
   }
 
   withLimitToLast(limit: number | null): Query {
-    const q = new Query(
+    return new Query(
       this.path,
       this.collectionGroup,
       this.explicitOrderBy.slice(),
@@ -178,7 +178,6 @@ export class Query {
       this.startAt,
       this.endAt
     );
-    return q;
   }
 
   withStartAt(bound: Bound): Query {

packages/firestore/src/core/sync_engine.ts

@@ -218,6 +218,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       viewSnapshot = queryView.view.computeInitialSnapshot();
     } else {
       const queryData = await this.localStore.allocateQuery(query);
+      // Make sure no query view is mapped yet. This happens when a 'mirror' query
+      // is already being listened to.
+      assert(
+        !this.queryViewsByTarget[queryData.targetId],
+        'Not Implemented: Mapping multiple distinct client queries to one backend query.'
+      );
       const status = this.sharedClientState.addLocalQueryTarget(
         queryData.targetId
       );

packages/firestore/src/local/local_store.ts

@@ -721,12 +721,6 @@ export class LocalStore {
           .getQueryData(txn, targetQuery)
           .next((cached: QueryData | null) => {
             if (cached) {
-              // Make sure target is not allocated to a mirror query already.
-              assert(
-                cached.query.isEqual(query),
-                'Not Implemented: Mapping multiple distinct client queries to one backend query.'
-              );
-
               // This query has been listened to previously, so reuse the
               // previous targetID.
               // TODO(mcg): freshen last accessed date?

packages/firestore/src/local/query_data.ts

@@ -38,6 +38,7 @@ export enum QueryPurpose {
 /**
  * An immutable set of metadata that the local store tracks for each query.
  */
+// TODO(wuandy): Rename this to TargetData?
 export class QueryData {
   constructor(
     /** The query being listened to. */

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

@@ -71,7 +71,7 @@ apiDescribe('Queries', (persistence: boolean) => {
       await expect(
         collection.limitToLast(2).get()
       ).to.be.eventually.rejectedWith(
-        'limitToLast() Query without explicit orderBy() is not supported yet.'
+        'limitToLast() queries require specifying an orderBy() on at least on document field.'
       );
     });
   });