-
Notifications
You must be signed in to change notification settings - Fork 605
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 15 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,28 @@ | |||||
|
||||||
package com.google.firebase.firestore.ktx | ||||||
|
||||||
import android.support.multidex.BuildConfig | ||||||
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 understand you needed this change to build it locally, but its not the correct Can you please remove this import line? 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. Good catch, that's done ✅ Turns out it wasn't even required for building. |
||||||
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 +172,67 @@ 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.
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( | ||||||
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. We are considering calling this docRef.documentSnapshot.collect { snapshot ->
// handle returned document
} I would like to hear y'all thoughts. 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. Not sure what the best practices are. It'd be worth investigating what other libs are doing. In all cases, we need to account for docRef.snapshots().collect { snapshot ->
// do something with snapshot
} And if customization is required, this becomes: docRef.snapshots(metadataChanges, bufferCapacity).collect { snapshot ->
// do something with 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. I'm with @martinbonnin here. It should be 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 agree it should be plural. 👍 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
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 went with just 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. |
||||||
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
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. Yup, sorry, just realized that. It's fixed in 692114c. 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. 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 commentThe 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 commentThe 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? |
||||||
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 | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Transforms a [Query] 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 | ||||||
* | ||||||
* @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 | ||||||
*/ | ||||||
martinbonnin marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
fun Query.toFlow( | ||||||
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. Same comment as above: we're considering calling 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. Same as here. I'm for 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
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. Don't forget to run |
||||||
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.
This is moved to
api
becauseFirebaseFirestoreException
exposes its supertypeFirebaseException
in 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
?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-firestore
module exposesFirebaseException
publicly 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:ktx
module 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