Release Query.limitToLast(n) #2321

wu-hui · 2019-11-01T17:23:49Z

Split Query and Target in the SDK.
Implement limitToLast.
Integration tests and spec tests for limitToLast.

… work with Target. (#2254)

…between Target and Query. (#2281)

* Add spec test for single client

… work with Target. (#2254)

…between Target and Query. (#2281)

* Add spec test for single client

…/firebase/firebase-js-sdk into wuandy/QueryTargetAndLimitToLast

schmidt-sebastian

Some remaining nits about the code comments, but this looks really great. Let's ship it (and then port it to Android and iOS)! 👍

schmidt-sebastian · 2019-11-01T17:45:45Z

packages/firestore/CHANGELOG.md

@@ -5,6 +5,8 @@
  `.where()`. `in` finds documents where a specified field’s value is IN a
  specified array. `array-contains-any` finds documents where a specified field
  is an array and contains ANY element of a specified array.
+- [feature] Added `limitToLast(n: number)` to Firestore query, which returns 


I would shorten this to:

Added Query.limitToLast(n: number), which returns the last n documents as the result.

schmidt-sebastian · 2019-11-01T17:48:42Z

packages/firestore-types/index.d.ts

+   * documents.
+   *
+   * Queries with `limitToLast` must have at least one `orderBy` clause on
+   * one of the document fields, or an Exception will throw during execution.


s/will throw/will be thrown/

schmidt-sebastian · 2019-11-01T17:48:57Z

packages/firebase/index.d.ts

+     * documents.
+     *
+     * You must specify at least one `orderBy` clause for `limitToLast` queries,
+     * otherwise an exception will be thrown thrown during execution.


s/thrown thrown/thrown/

schmidt-sebastian · 2019-11-01T17:50:24Z

packages/firestore/src/core/query.ts

  private memoizedOrderBy: OrderBy[] | null = null;

+  // The corresponding `Target` of this `Query` instance.
+  private target: Target | null = null;


Maybe rename this to memoizedTarget?

schmidt-sebastian · 2019-11-01T17:52:08Z

packages/firestore/src/core/sync_engine.ts

@@ -301,6 +307,18 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    const queryView = this.queryViewsByQuery.get(query)!;
    assert(!!queryView, 'Trying to unlisten on query not found:' + query);

+    // If it's not the only query mapped to the target, only clean up
+    // query view and keep the target active.


This would read better as:

"Only clean up the query view and target if this is the only query mapped to the target."

schmidt-sebastian · 2019-11-01T17:53:43Z

packages/firestore/src/core/sync_engine.ts

+   *
+   * The synthesized result might be different from the original `Query`, but
+   * since the synthesized `Query` should return the same results as the
+   * original one (only the presentation of results might differ), and this is


Suggest to drop and this is only used for multi-tab for readability. The porting note should be sufficient.

schmidt-sebastian · 2019-11-01T17:54:36Z

packages/firestore/src/local/shared_client_state.ts

@@ -110,6 +110,9 @@ export interface SharedClientState {
   * Associates a new Query Target ID with the local Firestore client. Returns
   * the new query state for the query (which can be 'current' if the query is
   * already associated with another tab).
+   *
+   * If the target id is already associated with local client, the method simply
+   * returns it's `QueryTargetState`.


s/it's/its/

schmidt-sebastian · 2019-11-01T17:55:02Z

packages/firestore/src/remote/remote_store.ts

-  /** Starts new listen for the given query. Uses resume token if provided */
+  /**
+   * Starts new listen for the given query. Uses resume token if provided. It
+   * is a no-op if the target of given `QueryData` is already being listened.


s/listened/listened to/

wu-hui

Thanks for all the reviews!!

wu-hui · 2019-11-01T19:12:57Z

packages/firebase/index.d.ts

+     * documents.
+     *
+     * You must specify at least one `orderBy` clause for `limitToLast` queries,
+     * otherwise an exception will be thrown thrown during execution.


wu-hui · 2019-11-01T19:13:03Z

packages/firestore-types/index.d.ts

+   * documents.
+   *
+   * Queries with `limitToLast` must have at least one `orderBy` clause on
+   * one of the document fields, or an Exception will throw during execution.


wu-hui · 2019-11-01T19:13:55Z

packages/firestore/CHANGELOG.md

@@ -5,6 +5,8 @@
  `.where()`. `in` finds documents where a specified field’s value is IN a
  specified array. `array-contains-any` finds documents where a specified field
  is an array and contains ANY element of a specified array.
+- [feature] Added `limitToLast(n: number)` to Firestore query, which returns 


wu-hui · 2019-11-01T19:15:15Z

packages/firestore/src/core/query.ts

  private memoizedOrderBy: OrderBy[] | null = null;

+  // The corresponding `Target` of this `Query` instance.
+  private target: Target | null = null;


wu-hui · 2019-11-01T19:16:31Z

packages/firestore/src/core/sync_engine.ts

@@ -301,6 +307,18 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    const queryView = this.queryViewsByQuery.get(query)!;
    assert(!!queryView, 'Trying to unlisten on query not found:' + query);

+    // If it's not the only query mapped to the target, only clean up
+    // query view and keep the target active.


wu-hui · 2019-11-01T19:17:29Z

packages/firestore/src/core/sync_engine.ts

+   *
+   * The synthesized result might be different from the original `Query`, but
+   * since the synthesized `Query` should return the same results as the
+   * original one (only the presentation of results might differ), and this is


wu-hui · 2019-11-01T19:18:13Z

packages/firestore/src/local/shared_client_state.ts

@@ -110,6 +110,9 @@ export interface SharedClientState {
   * Associates a new Query Target ID with the local Firestore client. Returns
   * the new query state for the query (which can be 'current' if the query is
   * already associated with another tab).
+   *
+   * If the target id is already associated with local client, the method simply
+   * returns it's `QueryTargetState`.


wu-hui · 2019-11-01T19:18:39Z

packages/firestore/src/remote/remote_store.ts

-  /** Starts new listen for the given query. Uses resume token if provided */
+  /**
+   * Starts new listen for the given query. Uses resume token if provided. It
+   * is a no-op if the target of given `QueryData` is already being listened.


wu-hui · 2019-11-02T01:20:46Z

@Feiyang1 This PR adds a new limitToLast() method to firestore queries. Please see the API review doc here: go/limitToLast

Can you or someone from your team provide an owner approval on index.d.ts ?

Feiyang1

Approved for the d.ts file changes.

wu-hui and others added 13 commits October 20, 2019 20:06

[1/3] Query/Target split -- create Target and make remote/ and local/…

9cab57c

… work with Target. (#2254)

Merge branch 'master' into wuandy/QueryTargetAndLimitToLast

42dc051

[2/3] Query/Target split: make SyncEngine able to handle the mapping …

ca58a29

…between Target and Query. (#2281)

[2.8/3] Add limitToLast implementation and tests. (#2296)

aa9390e

[3/3] Spec tests for limitToLast+multi-tab (#2300)

db3200a

Add spec test for single client (#2313)

bfa5764

* Add spec test for single client

[1/3] Query/Target split -- create Target and make remote/ and local/…

647a748

… work with Target. (#2254)

[2/3] Query/Target split: make SyncEngine able to handle the mapping …

1cd4fcf

…between Target and Query. (#2281)

[2.8/3] Add limitToLast implementation and tests. (#2296)

a7e96c2

[3/3] Spec tests for limitToLast+multi-tab (#2300)

603ced8

Add spec test for single client (#2313)

9ea2df7

* Add spec test for single client

Merge branch 'wuandy/QueryTargetAndLimitToLast' of https://github.com…

74a14de

…/firebase/firebase-js-sdk into wuandy/QueryTargetAndLimitToLast

add change log for limitToLast

5242336

wu-hui requested review from Feiyang1, hiranya911 and hsubox76 as code owners November 1, 2019 17:23

wu-hui requested a review from schmidt-sebastian November 1, 2019 17:24

wu-hui assigned schmidt-sebastian Nov 1, 2019

wu-hui added the api: firestore label Nov 1, 2019

schmidt-sebastian approved these changes Nov 1, 2019

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 1, 2019

Address comments

7d2aeef

wu-hui commented Nov 1, 2019

View reviewed changes

wu-hui assigned Feiyang1 and unassigned wu-hui Nov 2, 2019

Feiyang1 approved these changes Nov 2, 2019

View reviewed changes

Feiyang1 assigned wu-hui and unassigned Feiyang1 Nov 2, 2019

wu-hui merged commit 3323467 into master Nov 2, 2019

hsubox76 added this to the next milestone Nov 4, 2019

firebase locked and limited conversation to collaborators Dec 2, 2019

Release Query.limitToLast(n) #2321

Release Query.limitToLast(n) #2321

Uh oh!

Conversation

wu-hui commented Nov 1, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui commented Nov 2, 2019

Uh oh!

Feiyang1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!