feat: add support for Query.limitToLast() #954
Conversation
googlebot
added
the
cla: yes
schmidt-sebastian
force-pushed
the
mrschmidt/limittolast
branch
from
7299065
to
2529a5f
Compare
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/limittolast
branch
from
2529a5f
to
c9cc2b5
Compare
Codecov Report
@@ Coverage Diff @@
## master #954 +/- ##
==========================================
- Coverage 97.54% 97.45% -0.09%
==========================================
Files 25 25
Lines 15896 15983 +87
Branches 1268 1208 -60
==========================================
+ Hits 15506 15577 +71
- Misses 387 403 +16
Partials 3 3
Continue to review full report at Codecov.
|
schmidt-sebastian
force-pushed
the
mrschmidt/limittolast
branch
3 times, most recently
from
a3c23b9
to
8797f8e
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/limittolast
branch
from
8797f8e
to
b50874f
Compare
| @@ -969,6 +969,15 @@ interface QueryCursor { | |||
| values: unknown[]; | |||
| } | |||
|
|
|||
| /*! | |||
There was a problem hiding this comment.
Ultranit: should it be /**?
There was a problem hiding this comment.
"/?*" in JSDoc hides the comments from the public docs.
dev/src/reference.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Creates and returns a new Query that only returns the last matching |
There was a problem hiding this comment.
We should update limit()'s comment to mirror the wording there as well.
dev/src/reference.ts
Outdated
| }); | ||
|
|
||
| // Swap the cursors to match the now-flipped query ordering. | ||
| structuredQuery.startAt = this.toCursor( |
There was a problem hiding this comment.
I'd suggest keeping the cursor reverse logic here instead of inside toCursor, because the order flipping is also done in this block and that makes the intention more explicit IMHO, and that is also how we do it in other SDKs.
There was a problem hiding this comment.
I am not a huge fan of the code this creates, but it makes sense to keep in aligned with the rest of our SDKs.
dev/src/reference.ts
Outdated
| @@ -1898,13 +1993,13 @@ export class Query<T = DocumentData> { | |||
| } | |||
|
|
|||
| /** | |||
| * Internal streaming method that accepts an optional transaction id. | |||
| * Internal streaming method that accepts the requets proto. | |||
| @@ -903,6 +903,21 @@ declare namespace FirebaseFirestore { | |||
| * @return The created Query. | |||
| */ | |||
| limit(limit: number): Query<T>; | |||
|
|
|||
| /** | |||
| * Creates and returns a new Query that only returns the last matching | |||
There was a problem hiding this comment.
Same comment about bringing limit()'s comment in the same wording.
There was a problem hiding this comment.
Also done here.
| @@ -1079,6 +1079,18 @@ describe('Query class', () => { | |||
| }); | |||
| }; | |||
|
|
|||
| function addDocs(...data: DocumentData[]): Promise<WriteResult[]> { | |||
| @@ -1820,6 +1794,29 @@ describe('Query class', () => { | |||
| unsubscribe(); | |||
| }); | |||
| }); | |||
|
|
|||
| it('orders limitToLast() correctly', async () => { | |||
There was a problem hiding this comment.
Can this be moved to other limitToLast tests?
There was a problem hiding this comment.
This test is an integration test (in system-test/) and thus cannot be moved.
dev/test/query.ts
Outdated
|
|
||
| it('expects number', () => { | ||
| const query = firestore.collection('collectionId'); | ||
| expect(() => query.limit(Infinity)).to.throw( |
There was a problem hiding this comment.
should this be limitToLast instead?
Fixes #945
This adds support for limitToLast(), including order reserving for queries. Fortunately, Watch uses its own comparator, so we don't have to reverse the results in Watch.