feat(ui_firestore): add `includeMetadataChanges` parameter to FirestoreQueryBuilder

cpunion commented 5 months ago

Description

Currently FirestoreListView can't get metadata changes, the FirestoreQueryBuilder component calls Query<Document>.snapshots() without includeMetadataChanges parameter.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

[x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
[ ] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
[x] All existing and new tests are passing.
[x] I updated/added relevant documentation (doc comments with ///).
[x] The analyzer (melos run analyze) does not report any problems on my PR.
[x] All unit tests pass (melos run test:unit:all doesn't fail).
[x] I read and followed the Flutter Style Guide.
[x] I signed the CLA.
[x] I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

[ ] Yes, this is a breaking change.
[x] No, this is not a breaking change.

cpunion commented 5 months ago

I am not sure if there is a way to directly specify includeMetadataChanges when constructing the query. If there is such a method, then this PR is unnecessary.

russellwheatley commented 5 months ago

@cpunion - This seems good to me. I think we should also update here: https://github.com/firebase/FirebaseUI-Flutter/blob/main/packages/firebase_ui_firestore/lib/src/table_builder.dart#L267

Probably should be able to pass it into FirestoreDataTable if you could update, please?

cpunion commented 5 months ago

@cpunion - This seems good to me. I think we should also update here: https://github.com/firebase/FirebaseUI-Flutter/blob/main/packages/firebase_ui_firestore/lib/src/table_builder.dart#L267

Probably should be able to pass it into FirestoreDataTable if you could update, please?

There isn't any listen call, so I think it need a big change if add includeMetadataChanges?

russellwheatley commented 5 months ago

@cpunion - We can leave that for another time. Do you mind adding a simple test that passes in includeMetadataChanges and checks that metadata is passed in with snapshot here: https://github.com/firebase/FirebaseUI-Flutter/blob/main/tests/integration_test/firebase_ui_firestore/firestore_list_view_test.dart

Thanks 😄

cpunion commented 4 months ago

@cpunion - We can leave that for another time. Do you mind adding a simple test that passes in includeMetadataChanges and checks that metadata is passed in with snapshot here: https://github.com/firebase/FirebaseUI-Flutter/blob/main/tests/integration_test/firebase_ui_firestore/firestore_list_view_test.dart

I will try to add a test.

cpunion commented 4 months ago

@russellwheatley It seems that metadata changes too quickly to be captured by tester.pump*. For example, when creating and updating a document, collection.snapshots().listen(includeMetadataChanges: true) will receive a series of events in a short period of time, like this:

// These are the events from collection.add({value: 1})
received event, hasPendingWrites: true, isFromCache: true, data: ({value: 1})
received event, hasPendingWrites: true, isFromCache: false, data: ()
received event, hasPendingWrites: false, isFromCache: false, data: ()

// These are the events from collection.doc('docId').update({value: 2})
received event, hasPendingWrites: true, isFromCache: false, data: ({value: 2})
received event, hasPendingWrites: false, isFromCache: false, data: ()

When the FirestoreQueryBuilder listens to the collection, multiple setState calls occur in a very short time. tester.pump* only catches the last one, and can't catches changes of hasPendingWrites and isFromCache. I'm not sure how to properly test this case. Could you please help me? Thanks.

russellwheatley commented 6 days ago

@cpunion - could you show me what test you wrote? You can use a Completer to wait for something to return. E.g: https://github.com/firebase/flutterfire/blob/main/packages/cloud_firestore/cloud_firestore/example/integration_test/web_snapshot_listeners.dart#L36

cpunion commented 5 days ago

@russellwheatley I just added debug prints at https://github.com/firebase/FirebaseUI-Flutter/pull/349/files#diff-df489d5e040c259b77c51b59326d57ffa8ebbf45b0bee42e7bc1d257ab3ea68eR183 , so I think that is not testable.

firebase / FirebaseUI-Flutter