docs: Write javadocs for COUNT API #1783
Conversation
dconeybe
added
the
api: firestore
dconeybe
force-pushed
the
dconeybe/CountApiDocs
branch
from
fdb8d61
to
bd210d2
Compare
dconeybe
added
the
kokoro:force-run
dconeybe
added
kokoro:force-run
dconeybe
removed
the
kokoro:force-run
dev/src/reference.ts
Outdated
| field.isEqual(otherAggregates[alias]) | ||
| ) | ||
| ); | ||
| // Verify that this object has the same aggregation keys as `other`. |
There was a problem hiding this comment.
Can we do this without sort? Maybe there exists or we can create a helper method?
The consequence is likely small since we are likely only dealing with maps of one element. So, maybe we can just have a TODO to revisit this when other aggregate types are implement.
There was a problem hiding this comment.
I looked a little harder (I must have missed this while coding late last night!) and there already exists a deepEqual() function that employs the logic you suggested. So I've modified AggregateQuery.isEqual() and AggregateQuerySnapshot.isEqual() to just use deepEqual().
If interested, here is the raw source code of deepEqual(): https://github.com/epoberezkin/fast-deep-equal/blob/master/src/index.jst, and here is the compiled JavaScript version: https://gist.github.com/dconeybe/c2654f622b87fc0d4c75e5d58364491f
dev/src/reference.ts
Outdated
| if (!(other instanceof AggregateQuerySnapshot)) { | ||
| return false; | ||
| } | ||
| if (!this.readTime.isEqual(other.readTime)) { |
There was a problem hiding this comment.
The isEquals method for QuerySnapshot does not include timestamp. Are you sure this is correct behavior?
There was a problem hiding this comment.
Good point. DocumentSnapshot is actually a better class for comparison than QuerySnapshot, and it indeed explicitly excludes checking the read time:
nodejs-firestore/dev/src/document.ts
Lines 539 to 540 in ca36a44
I've removed the comparison of readTime, as suggested, to match the behavior of DocumentSnapshot.isEqual(). I've also updated the documentation, and will make the same fix in the java-firestore SDK. Good catch!
There was a problem hiding this comment.
For reference, here is the corresponding fix in the java-firestore SDK: googleapis/java-firestore#1051
dev/src/reference.ts
Outdated
|
|
||
| const otherData = other._data; | ||
| const otherDataKeys: string[] = Object.keys(otherData); | ||
| // Verify that this object has the same aggregation keys as `other`. |
There was a problem hiding this comment.
How is this better than previous map comparison?
There was a problem hiding this comment.
It's probably not better. I had mistakenly thought that sorting the keys is required. I've changed it to use deepEqual() now.
The wording is (mostly) copied from firebase/firebase-android-sdk#4143, and also partially from firebase/firebase-js-sdk#6632.
There were also some last-minute API and implementation changes:
AggregateField.count()andAggregateField.isEqual()were deleted, since they have no use case until other aggregations are supported (the same change was made in the firebase-js-sdk).Query._aggregate()was removed, since it was so much simpler to just inline it intoQuery.count().eslint-disable-next-linecomments.AggregateQuery.isEqual()andAggregateQuerySnapshot.isEqual()were tweaked to accommodate the deletion ofAggregateField.isEqual().