firebase-kotlin-sdk icon indicating copy to clipboard operation
firebase-kotlin-sdk copied to clipboard

Add Asynchronouts/Deferred implementation to Firestore/Database/etc

Open Daeda88 opened this issue 10 months ago • 5 comments

Wanted to start a discussion on this, so writing a ticket instead of a full on PR for a change. A suggestion for an implementation of this was originally in #448 but was removed because I couldn't properly clarify it at the time (someone else within our company had written it) and I felt it was best to take it out of the PR for clarity.

Consider the following usecase:

  • We have a (multiplatform) viewmodel with a coroutineScope that lives while the corresponding view is on screen
  • It receives frequent data from some service that needs to be stored to firestore, but we don't actually need to display whether that operation succeeded
  • Speed is important cause it's a lot of data

Then

  1. If I want to write the data fast, I can't use a collect on the flow of data to then send each data to firestore. Because it is a suspend method, I'll have to wait until the previous data was written before I can write the next data
  2. Alternatively, within the collect I can launch and write from there, but launch takes some time to spin up and may slow me down.
  3. Since I'm launching with the VM scope, the collect and therefore the write will stop when I leave the view (assuming I'm not able to put it in a service that survives the screen for whatever reason).

To solve this, it could be considered to add a non-suspended (deferred) approach to the library. Where the methods are not suspended at all, but just return a Deferred of the result. Since the platform APIs do nothing with coroutines, this is very straightforward.

It may be argued that you can just call the suspended methods within scope.async {} but that means you need to have a coroutineScope, which adds a lot of overhead for high volumes of data and has some form of a lifecycle. Or, to look at it from another way: even though the firebase implementation is suspended, since the internal implementation is not, actions like cancelling the scope have little effect on the interaction with the internal firebase sdk. So from both perspectives, coroutineScopes are not strictly required nor necessarily even beneficial.

My suggestion boils down to this.

Current code:

@PublishedApi
internal expect class NativeDocumentReference(nativeValue: NativeDocumentReferenceType) {
    suspend fun setEncoded(encodedData: EncodedObject, setOptions: SetOptions)
}

data class DocumentReference internal constructor(@PublishedApi internal val native: NativeDocumentReference) {
    suspend inline fun <reified T : Any> set(data: T, merge: Boolean = false, buildSettings: EncodeSettings.Builder.() -> Unit = {}) = native.setEncoded(
        encodeAsObject(data, buildSettings), if (merge) SetOptions.Merge else SetOptions.Overwrite)

}

New code:

@PublishedApi
internal expect class NativeDocumentReference(nativeValue: NativeDocumentReferenceType) {
    fun setEncoded(encodedData: EncodedObject, setOptions: SetOptions): Deferred<Unit>
}

data class DocumentReference internal constructor(@PublishedApi internal val native: NativeDocumentReference) {

    data class Async(@PublishedApi internal val native: NativeDocumentReference) {
         inline fun <reified T : Any> set(data: T, merge: Boolean = false, buildSettings: EncodeSettings.Builder.() -> Unit = {}) = native.setEncoded(
            encodeAsObject(data, buildSettings), if (merge) SetOptions.Merge else SetOptions.Overwrite)
        }
    }
    
    val async = Async(native)

    suspend inline fun <reified T : Any> set(data: T, merge: Boolean = false, buildSettings: EncodeSettings.Builder.() -> Unit = {}) = async.set(data, merge, buildSettings).await()
}

If others see the benefits of adding an async implementation as well, Im happy to make a PR for it.

Daeda88 avatar Apr 16 '24 09:04 Daeda88