-
Notifications
You must be signed in to change notification settings - Fork 615
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 20 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 | ||||
---|---|---|---|---|---|---|
|
@@ -18,14 +18,23 @@ 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.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 +171,73 @@ class FirebaseFirestoreKtxRegistrar : ComponentRegistrar { | |||||
override fun getComponents(): List<Component<*>> = | ||||||
listOf(LibraryVersionComponent.create(LIBRARY_NAME, BuildConfig.VERSION_NAME)) | ||||||
} | ||||||
|
||||||
/** | ||||||
* Starts listening to the document referenced by this `DocumentReference` with the given options and emits its values via a [Flow]. | ||||||
* | ||||||
* - When the returned flow starts being collected, an [EventListener] will be attached. | ||||||
* - When the flow completes, the listener will be removed. | ||||||
* | ||||||
* Backpressure: by default the returned flow is conflated. If the consumer isn't fast enough, | ||||||
* it might miss some values but is always guaranteed to get the latest value. Use the [bufferCapacity] parameter | ||||||
* to change that behaviour. | ||||||
* | ||||||
* @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.snapshots( | ||||||
metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | ||||||
bufferCapacity: Int? = Channel.CONFLATED | ||||||
): Flow<DocumentSnapshot> { | ||||||
val flow = callbackFlow { | ||||||
val registration = addSnapshotListener(metadataChanges) { snapshot, exception -> | ||||||
if (exception != null) { | ||||||
cancel(message = "Error getting DocumentReference snapshot", cause = exception) | ||||||
} else if (snapshot != null) { | ||||||
trySend(snapshot) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Along with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see my response re
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 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I've pushed a change to use 2 questions though:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would love to hear thoughts from the firestore folks on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good 👍 |
||||||
} | ||||||
} | ||||||
awaitClose { registration.remove() } | ||||||
} | ||||||
|
||||||
return if (bufferCapacity != null) { | ||||||
flow.buffer(bufferCapacity) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Please see my comment here and previous comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with this if it is documented as you suggested. |
||||||
} else { | ||||||
flow | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Starts listening to this query with the given options and emits its values via a [Flow]. | ||||||
* | ||||||
* - When the returned flow starts being collected, an [EventListener] will be attached. | ||||||
* - When the flow completes, the listener will be removed. | ||||||
* | ||||||
* Backpressure: by default the returned flow is conflated. If the consumer isn't fast enough, | ||||||
* it might miss some values but is always guaranteed to get the latest value. Use the [bufferCapacity] parameter | ||||||
* to change that behaviour. | ||||||
* | ||||||
* @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 Query.snapshots( | ||||||
metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
bufferCapacity: Int? = Channel.CONFLATED | ||||||
): Flow<QuerySnapshot> { | ||||||
val flow = callbackFlow { | ||||||
val registration = addSnapshotListener(metadataChanges) { snapshot, exception -> | ||||||
if (exception != null) { | ||||||
cancel(message = "Error getting Query snapshot", cause = exception) | ||||||
} else if (snapshot != null) { | ||||||
trySend(snapshot) | ||||||
} | ||||||
} | ||||||
awaitClose { registration.remove() } | ||||||
} | ||||||
|
||||||
return if (bufferCapacity != null) { | ||||||
flow.buffer(bufferCapacity) | ||||||
} else { | ||||||
flow | ||||||
} | ||||||
} |
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.
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?