-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[cloud_firestore] Add metadata to QuerySnapshot #1915
Conversation
collinjackson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to update CHANGELOG and pubspec before merging.
| stream: firestore.collection('messages').snapshots(), | ||
| builder: (BuildContext context, AsyncSnapshot<QuerySnapshot> snapshot) { | ||
| if (!snapshot.hasData) return const Text('Loading...'); | ||
| print(snapshot.data.metadata.isFromCache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest removing these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops that was not supposed to make it into the PR, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| /// is the first snapshot, all documents will be in the list as Added changes. | ||
| final List<DocumentChange> documentChanges; | ||
|
|
||
| final SnapshotMetadata metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to add unit testing and a basic integration test for this, right? Even just asserting that hasPendingWrites and isFromCache aren't null would be better than nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I checked that the metadata property of the QuerySnapshot was not null.
66deb36 to
8748653
Compare
| @@ -1,3 +1,7 @@ | |||
| ## 0.12.8+1 | |||
|
|
|||
| * Add metatdata to QuerySnapshot. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Add metatdata to QuerySnapshot. | |
| * Add `metadata` to `QuerySnapshot`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
Once tests pass I'll merge and publish. |
c9c8d61 to
99d6275
Compare
|
This looks ready to merge, thanks! |
* Add metadata to QuerySnapshot
* Add metadata to QuerySnapshot
Description
Bring
cloud_firestoreplugin in line with native SDKs by exposing the metadata of QuerySnapshot.Related Issues
Fixes: flutter/flutter#35550