-
Notifications
You must be signed in to change notification settings - Fork 662
Add 'toFlow()' extensions to DocumentSnapshot and Query #1252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
af0f469
931e73f
57fbe74
c87ea63
fe0916c
15bb8b9
fcfb4b1
ad9db4d
4d7237d
de32290
cccf86f
c91731a
a3d04ac
31d1553
2e51a63
288ae9a
bcb69e5
f6e1aeb
fbd4864
31d3b61
8cd52cd
253fbca
692114c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,18 +14,29 @@ | |||||
|
|
||||||
| package com.google.firebase.firestore.ktx | ||||||
|
|
||||||
| import android.support.multidex.BuildConfig | ||||||
|
||||||
| import androidx.annotation.Keep | ||||||
| import com.google.firebase.FirebaseApp | ||||||
| import com.google.firebase.components.Component | ||||||
| import com.google.firebase.components.ComponentRegistrar | ||||||
| import com.google.firebase.firestore.DocumentReference | ||||||
| import com.google.firebase.firestore.DocumentSnapshot | ||||||
| import com.google.firebase.firestore.FieldPath | ||||||
| import com.google.firebase.firestore.FirebaseFirestore | ||||||
| import com.google.firebase.firestore.FirebaseFirestoreSettings | ||||||
| import com.google.firebase.firestore.MetadataChanges | ||||||
| import com.google.firebase.firestore.Query | ||||||
| import com.google.firebase.firestore.QueryDocumentSnapshot | ||||||
| import com.google.firebase.firestore.QuerySnapshot | ||||||
| import com.google.firebase.ktx.Firebase | ||||||
| import com.google.firebase.platforminfo.LibraryVersionComponent | ||||||
| import kotlinx.coroutines.CancellationException | ||||||
| import kotlinx.coroutines.cancel | ||||||
| import kotlinx.coroutines.channels.Channel | ||||||
| import kotlinx.coroutines.channels.awaitClose | ||||||
| import kotlinx.coroutines.flow.Flow | ||||||
| import kotlinx.coroutines.flow.buffer | ||||||
| import kotlinx.coroutines.flow.callbackFlow | ||||||
|
|
||||||
| /** Returns the [FirebaseFirestore] instance of the default [FirebaseApp]. */ | ||||||
| val Firebase.firestore: FirebaseFirestore | ||||||
|
|
@@ -162,3 +173,71 @@ class FirebaseFirestoreKtxRegistrar : ComponentRegistrar { | |||||
| override fun getComponents(): List<Component<*>> = | ||||||
| listOf(LibraryVersionComponent.create(LIBRARY_NAME, BuildConfig.VERSION_NAME)) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Transforms a [DocumentReference] into a coroutine [Flow] | ||||||
| * | ||||||
| * **Backpressure handling**: by default this method conflates items. If the consumer isn't fast enough, | ||||||
| * it might miss some values but is always guaranteed to get the latest value. Use [bufferCapacity] | ||||||
| * to change that behaviour | ||||||
martinbonnin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| * | ||||||
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||||||
| * @param bufferCapacity the buffer capacity as in [Flow.buffer] or null to not buffer at all | ||||||
| */ | ||||||
| fun DocumentReference.toFlow( | ||||||
|
||||||
| fun DocumentReference.toFlow( | |
| fun DocumentReference.documentSnapshots( |
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 went with just snapshots() in this commit. As @svenjacobs said, documentSnapshots() is more verbose and the "document" part is already in the "documentRef". Let me know if you want me to change it or feel free to amend.
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.
Sounds good to me.
Outdated
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.
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | |
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE |
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.
Yup, sorry, just realized that. It's fixed in 692114c.
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.
Also interestingly this seemed to be a lint-only issue as Kotlin 1.4 is perfectly happy with the trailing coma
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.
yeah, I think our version of ktlint is pretty ancient, we are planning to upgrade it eventually
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.
@vkryachko I think all comments have been addressed. Can you please approve the pending CI workflows and review the PR once again?
Outdated
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.
shouldn't it be trySendBlocking instead? here and below
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.
Along with buffer this should be safe and not block the UI thread. Please see my comment here.
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.
see my response re buffer, as for the comment you linked:
@martinbonnin Why not just use trySend instead of risking blocking the thread, which is likely to be the main one?
imo the fact that Firestore delivers events on the UI thread is an unfortunate(but necessary) historic artifact as it optimized for devEx before coroutines were available and switching to the UI thread was non-trivial, and most devs want to render UI when data changes so UI thread was chosen. However Firestore provides overloads for addSnapshotListener that takes an executor. Now in the coroutine and flow world, devs have explicit control of what thread they are collecting on, so we can afford to deliver events to the flow on any thread we like.
So I think it's a safer choice to be as unopinionated as possible and use a safe executor + trySendBlocking, i.e. AsyncTask.THREAD_POOL_EXECUTOR or com.google.firebase.firestore.util.Executors.BACKGROUND_EXECUTOR(which is more limited).
This will make our code safe by default, i.e. we will not "drop" events with CONFLATED, and given direct control to developers. wdyt?
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.
Sounds good to me
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've pushed a change to use BACKGROUND_EXECUTOR.
2 questions though:
- Isn't this the equivalent of
buffer(UNLIMITED)in practice? It's not using the coroutinesbuffer()but I think it will enqueueRunnables in the executors until the consumer can consume them? So all in all it's also doing a choice for the user. - Is there any executor that could avoid a thread switch (immediate maybe?) ? I'm not familiar with the Firestore internals but if the code is running in a background thread already, feels like there's no need to dispatch?
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.
- Yeah, I guess you're right, it may be suboptimal in this case. so direct executor sounds more appealing.
- That's a good question, on the one hand I like the idea of using direct executor but I am not sure where the execution happen in that case. grpc io pool maybe? what is the risk of starving those threads in that case? it may not actually be an issue since the developer have full control and can avoid congestion of that pool.
Would love to hear thoughts from the firestore folks on this.
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 chatted with Firestore folks offline, given that Firestore is single-threaded internally, they really don't want to run arbitrary user code on their internal thread, so BACKGROUND_EXECUTOR is preferred.
@martinbonnin thanks for accommodating all the changes
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.
Sounds good 👍
Outdated
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.
Do we need to have an opinion on this or should we just let the developer do their own buffering?
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.
Please see my comment here and previous comments. buffer is essential for back-pressure handling.
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.
While it's not unreasonable to do it, I just don't think firestore itself is in a position to choose a default buffering strategy and instead it should be the responsibility of the developer
i.e. I am not convinced that CONFLATED is a good default, what if the developer can't afford to lose any events and wants to get them all even if it means congesting the producer. Point being we don't know the use case, so we should not be opinionated about buffering and let the developer explicitly decide by using buffer().
imo it's safer to just add kdoc that recommends using buffering if needed.
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'm fine with this if it is documented as you suggested.
martinbonnin marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
Same comment as above: we're considering calling this Query.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.
Same as here. I'm for snapshots().
Outdated
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.
| fun Query.toFlow( | |
| fun Query.querySnapshots( |
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.
Don't forget to run ./gradlew :firebase-firestore:ktx:generateApiTxtFile after making these changes. That's the gradle task that generates our api.txt file.
Outdated
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.
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | |
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE |
Outdated
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.
| ): Flow<QuerySnapshot?> { | |
| ): Flow<QuerySnapshot> { |
Does it really need to be nullable? At the moment, trySend() is only called with non-null snapshots.
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.
Good point 👍 . Doc says
It's guaranteed that exactly one of value or error will be non-{@code null}.
So yay for making everything non-nullable, I'll push the change in a bit.
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.
This is moved to
apibecauseFirebaseFirestoreExceptionexposes its supertypeFirebaseExceptionin public APIThere 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.
Sorry, I didn't understand the reason for this change and how it relates to the Flows. Are you hoping to expose
FirebaseException?Uh oh!
There was an error while loading. Please reload this page.
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.
The
firebase-firestoremodule exposesFirebaseExceptionpublicly fromEventListener:Without this, I'm getting this error:
We could also add
implementation 'com.google.android.gms:play-services-basement:18.0.0'to the:firebase-firestore:ktxmodule but I believe the proposed change above is better as it will fix it for all consumers, even the non-ktx ones.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.
@martinbonnin Ah, I see. Thanks for explaining.
I would prefer adding that implementation to firebase-firestore-ktx instead of exposing it on the base module.
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 in f6e1aeb