Open Daeda88 opened 2 months ago
- Alternatively, within the collect I can launch and write from there, but launch takes some time to spin up and may slow me down.
What gives you the impression launch takes time to spin up?
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.
What scope would you await the Deferred returned from the proposed api? Doesn't that give you the same problem?
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).
Just launch it in the GlobalScope instead then? That's essentially what's happening if the API was to return Deferred.
Also since (at least for android) we are using Task<T>.await()
then according to the docs scope cancellation just means it stops waiting for the task instead of stopping the task (we should probably switch to using Task<T>.await(cancellationTokenSource: CancellationTokenSource)
)
inline fun <reifeid T> getDeferred(): Deferred<T> {
val result = CompletableDeferred<T>()
firestoreSDK.doStuff(object : FirestoreCallback {
override fun onSucces(value: T) {
result.complete(value)
}
override fun onError(error: Exception) {
result.completeExceptionally(error)
}
})
return result
}
Launching on Globalscope is a big no-no in my book, and its entirely unnecessary to launch anything if you want a deferred.
In the end, users wrapping the suspend methods in async
as would currently be the recommended strategy, instead of having the suspend methods wrap a deferred as this ticket proposes, adds unnecessary overhead that also requires either lifecycle management or launching in an non-recommended scope.
In the end, users wrapping the suspend methods in
async
as would currently be the recommended strategy, instead of having the suspend methods wrap a deferred as this ticket proposes, adds unnecessary overhead that also requires either lifecycle management or launching in an non-recommended scope.
I believe the unnecessary overhead is negligible and not worth the api bloat proposed here. Requiring lifecycle management is good thing - it forces developers to consider the lifetime of the async operation and is the reason for the whole coroutines scope concept.
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).
Just want to understand your usecase, it sounds like your job shouldn't be bound to the VM scope because you don't want it cancelling when the viewmodel scope is closed. For your scenario why cant you use do something like:
CoroutineScope(Dispatchers.IO).launch {
// stuff to do
}
I don't think its a no no, especially when you don't want it to be bound to a particular scope.
Assuming it is android, If you want to be super safe, you could create an application scope that is associated to your Application and use this instead?
I tend to agree with @nbransby here.
My experience is that writing to Firestore already is really fast, because afaik it defaults to writing to the local cache first and the synchronizes async. Write functions returns almost immediately, even when writing fails due to Rules or what not.
@Daeda88 how much data are we talking about here? 10 doc/s? 100 docs/s? More?
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:
Then
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:
New code:
If others see the benefits of adding an async implementation as well, Im happy to make a PR for it.