This PR intends to satisfy this Feature Request : https://github.com/firebase/firebase-android-sdk/issues/1251

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Hi @michgauz. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@googlebot I signed it!

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

/ok-to-test

@michgauz thanks for sending this! I saw your discussion about this on Android Study Group.

Just to set some expectation, all public API changes go through our internal API review process, so if @vkryachko or one of our other Kotlin enthusiasts else decides to push this forward with this it could take some time for them to write up the internal document and schedule the review meeting.

The public api surface has changed for the subproject firebase-firestore_ktx: error: Added method com.google.firebase.firestore.ktx.FirestoreKt.toFlow(com.google.firebase.firestore.DocumentReference) [AddedMethod] error: Added method com.google.firebase.firestore.ktx.FirestoreKt.toFlow(com.google.firebase.firestore.Query) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@wu-hui Would you be willing to draft the API proposal document for this change?

The public api surface has changed for the subproject firebase-firestore_ktx: error: Added method com.google.firebase.firestore.ktx.FirestoreKt.toFlow(com.google.firebase.firestore.DocumentReference) [AddedMethod] error: Added method com.google.firebase.firestore.ktx.FirestoreKt.toFlow(com.google.firebase.firestore.Query) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

The public api surface has changed for the subproject firebase-firestore_ktx: error: Added method com.google.firebase.firestore.ktx.FirestoreKt.toFlow(com.google.firebase.firestore.DocumentReference) [AddedMethod] error: Added method com.google.firebase.firestore.ktx.FirestoreKt.toFlow(com.google.firebase.firestore.Query) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

Codecov Report

Merging #1252 (2db160f) into master (052fc33) will decrease coverage by 8.00%. The diff coverage is 0.00%.

:exclamation: Current head 2db160f differs from pull request most recent head 692114c. Consider uploading reports for the commit 692114c to get more accurate results

@@             Coverage Diff              @@
##             master    #1252      +/-   ##
============================================
- Coverage     29.22%   21.21%   -8.01%     
============================================
  Files           203        1     -202     
  Lines          8333       33    -8300     
  Branches        787        4     -783     
============================================
- Hits           2435        7    -2428     
+ Misses         5714       26    -5688     
+ Partials        184        0     -184     

Flag	Coverage Δ
Encoders_FirebaseEncodersJson	?
Encoders_FirebaseEncodersReflective	?
FirebaseCrashlytics	?
FirebaseDatatransport	?
FirebaseFirestore_Ktx	21.21% <0.00%> (?)
FirebaseInappmessaging	?
FirebaseInappmessagingDisplay	?
FirebaseInappmessagingDisplay_Ktx	?
FirebaseInappmessaging_Ktx	?
Transport_TransportBackendCct	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lin/com/google/firebase/firestore/ktx/Firestore.kt	21.21% <0.00%> (ø)
.../internal/bindingwrappers/ImageBindingWrapper.java
...gle/firebase/inappmessaging/model/MessageType.java
...le/android/datatransport/cct/internal/QosTier.java
...ssaging/display/internal/PicassoErrorListener.java
...njection/modules/HeadlessInAppMessagingModule.java
...ernal/injection/modules/AnalyticsEventsModule.java
...base/crashlytics/internal/network/HttpRequest.java
.../inappmessaging/internal/DisplayCallbacksImpl.java
...ytics/internal/settings/SettingsJsonConstants.java
... and 195 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

I have just been aware of this:https://github.com/Kotlin/kotlinx.coroutines/issues/974

Long story short, channel.offer might throw and we might need to catch any exception thrown. It's not 100% clear to me if it's strictly required here since the firestore callbacks are run on the main thread so maybe that changes things but I'd still feel safer if we could catch them.

I have just been aware of this:Kotlin/kotlinx.coroutines#974

Long story short, channel.offer might throw and we might need to catch any exception thrown. It's not 100% clear to me if it's strictly required here since the firestore callbacks are run on the main thread so maybe that changes things but I'd still feel safer if we could catch them.

+1 The comments on that Issue make a very compelling case for dealing with the exception.

Another thing to consider it that the current implementation uses callbackFlow which is marked as @ExperimentalCoroutinesApi (ref) @samtstern @rosariopfernandes @vkryachko thoughts?

For reference, androidx.lifecycle usage of flow is limited to Flow's stable API in non-test code.

Another thing to consider it that the current implementation uses callbackFlow which is marked as @ExperimentalCoroutinesApi (ref) @samtstern @rosariopfernandes @vkryachko thoughts?

For reference, androidx.lifecycle usage of flow is limited to Flow's stable API in non-test code.

@michgauz thanks for the PR and sorry for the long wait! after talking with the team we've agreed that while callbackFlow is the way to go we should restrict ourselves to Flow's stable API.

Until callbackFlow is marked as stable, the pattern to use in Firebase to implement flows should be the same as the one used in androidx.lifecycle.FlowLiveData LiveData.asFlow() In short, using Channel and Observer instead.

@rlazo Sorry I missed your previous comment. I agree that sticking to the Stable APIs is better.

@michgauz digging into @rlazo 's suggestion of using Channel instead, I came up with the code below. PTAL and feel free to use it in your PR:

fun Query.toFlow(metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE): Flow<QuerySnapshot> = flow {
    val channel = Channel<QuerySnapshot>(Channel.CONFLATED)
    var listener: ListenerRegistration? = null
    withContext(Dispatchers.Main.immediate) {
        listener = addSnapshotListener(metadataChanges) { value, error ->
            value?.let { channel.offer(it) }
            error?.let { channel.close(it) }
        }
    }
    try {
        for (value in channel) {
            emit(value)
        }
    } finally {
        GlobalScope.launch {
            listener?.remove()
        }
    }
}

@rosariopfernandes You should really use callbackFlow. Your solution references GlobalScope which should be avoided. callbackFlow uses Channel internally. So you're basically reinventing the wheel 😉

GloblaScope.launch can be removed and just get the listener removal inside withContext(Dispatchers.Main.immediate + NonCancellable). I think that's an acceptable solution until callbackFlow stabilizes.

Sorry, I didn't notice that the usage of callbackFlow is a problem. But instead of trying to work around it, I would just mark the function as experimental, too, until callbackFlow becomes stable.

@svenjacobs ah, I see. Thanks for pointing that out!

@LouisCAD sounds good! Thanks

Hello :) Thanks for your feedbacks. I update the PR with the @rosariopfernandes feat @LouisCAD solution

Coverage Report ¹

Affected Products

• firebase-firestore-ktx

  Overall coverage changed from 100.00% (c6cc8c5) to 25.93% (95a4508) by -74.07%.

  Filename	Base (c6cc8c5)	Merge (95a4508)	Diff
  Firestore.kt	100.00%	25.93%	-74.07%

Test Logs

• Base (c6cc8c5): https://android-ci.firebaseopensource.com/view/gcs/android-ci/logs/postsubmit-check-coverage/1557066287392231424
• Merge (95a4508): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/1252/check-coverage-changed/1557717959189729283

Notes

• Commit (95a4508) is created by Prow via merging PR base commit (c6cc8c5) and head commit (692114c).
• Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

Size Report ¹

Affected Products

• firebase-firestore-ktx

  Type	Base (c6cc8c5)	Merge (95a4508)	Diff
  aar	7.43 kB	15.3 kB	+7.89 kB (+106.2%)
  apk (aggressive)	504 kB	507 kB	+2.74 kB (+0.5%)
  apk (release)	3.94 MB	4.31 MB	+370 kB (+9.4%)

Test Logs

• Base (c6cc8c5): https://android-ci.firebaseopensource.com/view/gcs/android-ci/logs/postsubmit-binary-size/1557066287388037123
• Merge (95a4508): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/1252/binary-size/1557717959189729281

Notes

• Commit (95a4508) is created by Prow via merging PR base commit (c6cc8c5) and head commit (692114c).

Good news, everyone! :smiley: callbackFlow is stable in Coroutines 1.5.0 :partying_face:

Any chance to get this merged ?

For the record, my current implementation using the now stable callbackFlow is

/**
 * Attach a snapshotListener to a [DocumentReference] and use it as a coroutine [Flow]
 * @param metadataChanges Indicates whether metadata-only changes
 */
fun DocumentReference.toFlow(metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE) = callbackFlow {
    val listener = addSnapshotListener(metadataChanges) { value, error ->
        if (value != null) {
            trySendBlocking(value)
        }
        if (error != null) {
            cancel(CancellationException("Error getting DocumentReference snapshot", error))
        }
    }

    awaitClose { listener.remove() }
}

/**
 * Attach a snapshotListener to a [Query] and use it as a coroutine [Flow]
 * @param metadataChanges Indicates whether metadata-only changes
 */
fun Query.toFlow(metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE): Flow<QuerySnapshot> = callbackFlow {
    val listener = addSnapshotListener(metadataChanges) { value, error ->
        if (value != null) {
            trySendBlocking(value)
        }
        if (error != null) {
            cancel(CancellationException("Error getting Query snapshot", error))
        }
    }

    awaitClose { listener.remove() }
}

@martinbonnin Why not just use trySend instead of risking blocking the thread, which is likely to be the main one?

@martinbonnin Why not just use trySend instead of risking blocking the thread, which is likely to be the main one?

Good question. I'm not sure TBH. I took inspiration from https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/callback-flow.html

Can we get this merged, please? 🙏🏼