https://kotlinlang.org logo
Join Slack
Powered by
What’s the proper way to inject the `CoroutineDisp...
# android
z
What’s the proper way to inject the 
CoroutineDispatcher
for 
LifecycleCoroutineScope
in production? Seems it’s hardcoded to 
Dispatchers.Main.immediate
and 
SupervisorJob
which is something I desire flexibility around https://developer.android.com/topic/libraries/architecture/coroutines#lifecyclescope
w
If you need that flexibility, maybe you don’t need the built in scope? What’s the scenario you need this for, out of curiosity?
z
we provide the ability to inject the dispatchers
so directly leveraging 
Dispatchers.Main.immediate
is a violation of that
w
we provide the ability to inject the dispatchers
Yes but why 😄
I mean, if you need that much control I suppose your best bet is to you have your own scope. Changing the one provided by Arch doesn’t seem like a good idea
z
maybe I want to inject a specific 
CoroutineName
for debugging purposes, who knows
f
You can inject dispatcher with creating custom annotation for each dispatcher and providing it if you are using dagger
👍 1
z
if you are using dagger
no
f
Ok :)
z
creating custom annotation for each dispatcher
Not using reflection, java 8, or annotation processing
w
z
setting static state has side effects, which makes any solution that leverages that unacceptable. For an individual fragment, I want to inject which dispatcher it uses. I don’t want to override a static one
It looks to me that the AndroidX team failed in this regard and has designed their code here in a way to prevent dependency injection and instead rely on static state and all of its problems
w
😄 Then why use provided view model extensions instead of writing your own?
z
once you have a reference to a 
Fragment
, you can inject your own 
ViewModel
.viewModelScope
is an anti-pattern because it prevents injecting a 
CoroutineScope
, so I don’t use it
like the workaround for enabling 
viewModelScope
to work when under test is a joke
but all the functions on 
LifecycleScope
are very useful, like 
launchWhenStarted
r
I think for most usecases viewModelScope ,will probably be enough.. its already tied to the lifecycle of the viewModel which is already lifecycle aware... As for testing purposes, one way to make testing easier is to create and utilize CoroutineScope extenstion functions or use coroutineScope Builder function. That way when you use runBlocking in your Tests, it always inherits the test scope... Or if you do need to create randomly excute a job withIn a function , just Dispatchers.setMain(...) works well with viewModelScope
z
@rkeazor by far the best way is to make testing easier is to leverage a Jetpack 
ViewModel
to construct your own non-jetpack 
ViewModel
(without 
onCleared()
) with a 
CoroutineScope
that is cancelled in the jetpack’s 
onCleared()
. Injecting a 
CoroutineScope
means you have no need to leverage 
onCleared()
because 
CoroutineScope
provides its own method for completion callback via 
CompletionHandler
. There’s no better, cleaner way to do it.
r
@zak.taccardi umm I don't know that sounds pretty strange to me. It just sounds like a extra layer of abstraction that isn't useful. The current implementation of ViewModel is pretty easy to test. You basically test it like any other class. You only need to leverage onCleared when your testing onCleared
z
you should be shutting down any coroutines that are running from your test when your test ends
r
yea but you don't have to do that manually
z
correct. it is automatically done for you when you inject the 
CoroutineScope
from 
runBlocking { .. }
r
yea but you dont need to inject coroutine scope to achieve that
for instance if I write a function
Copy code
fun CoroutineScope.addNumbers(...)
z
correct. you can achieve that with a bunch of crazy, unnecessary `@Rule`s that you apply to your tests
r
when I run this using runBlocking the scope is inherited from runBlocking
z
we’re talking about unit testing ViewModels
r
same as this
Copy code
suspend fun addNumnbers() { coroutineScope {
same
thing
z
.viewModelScope
ignores the 
runBlocking
you are wrapping your tests with because you violate structured concurrency
r
you only add the rules when your creating a job within a function
z
generally speaking if you are using coroutines, the only reason to directly access a 
Job
instance is when you are doing something very complex
````
r
So its not that viewModelScope in particular ignores runBlocking, is that there are times when you job is created outside the scope of runBlocking. This issue would happen regardless of viewModelScope
let me show you an example
👍 1
Copy code
fun addSomething(x: Int, y: Int) { 
launch { x + y  }
.....
}
this test will always fail if you use runBlocking
because you would need reference to the scope that is the parent of that coroutine
z
correct, which is why you should make the parent injectable
r
this is when you would need to set the main dispatcher
z
I don’t use the main dispatcher for my view models
(though that’s an implementation detail)
I use 
Dispatchers.Default
in production and the blocking dispatcher from 
runBlocking { .. }
when testing (or 
runBlockingTest(..)
)
My ViewModels don’t care because they are abstracted from it via the 
CoroutineScope
injection
Effective Java Item 1 is to favor composition over inheritance, and that is what injecting a 
CoroutineScope
into a class over extending Jetpack’s 
ViewModel
achieves
r
but see that issue isn't a issue with viewmodel, its just a logical complexity coroutine scope, but you can always use the coroutine scope builder and you will never see that issue
but in the end your still doing some level of inheritance, because your utilizing viewModel.
its just a extra layer of abstraction
if viewModel is already testable , I would argue that its easier to add a rule than boilerplate abstraction ,
a rule is like one line
z
My view models are complex pieces of code. I reduce that complexity by injecting 
CoroutineScope
. That 
CoroutineScope
is then provided via the actual Jetpack 
ViewModel
, which is abstracted via a single function call. This function is a standalone module that uses the Jetpack 
ViewModel
as an implementation dependency so the rest of the codebase is not coupled to the Jetpack `ViewModel
a rule is like one line
One line per test. And if you forget to use it, test fails. Also breaks the ability to execute tests in parallel because you now rely on shared static state
r
so what life-cycle do you tie your coroutine scope to ?
yea but testing rules aren't a bad thing. Its the devs responibility that they don't forget that lie.... But abstracting a ViewModel, to another ViewModel poses more trouble
z
it depends on where my non-jetpack 
MyViewModel
is scoped to. I usually scope to user flows rather than UI components like Fragments because then I can’t share that 
MyViewModel
with other fragments. But this is abstracted by how the 
MyViewModel
is provided
r
First people joining your team won't know whats going on, because that architecture isn't synthetic to any architecture they would be accustom to. Two the Amount of BoilerPlate vs minmum gain , just doesn't match up
z
there’s no boilerplate
not sure what you are talking about
r
to create a viewModel , from jetpack viewmodel, there has to be some type of boilerplate
your writing more code somewhere
and that extract code is boilerplate. even if you extract it out to base class and use generics
z
it’s the same as the existing jetpack view model boilerplate, but instead of a factory function that has a single parameter of 
Class<T>
, it’s 
Class<T>
+ 
CoroutineScope
r
and all this is being done because to avoid a one line test rule
z
one line in 50 test classes is 50 lines
r
lol make a base test lol
it is still just one line
z
why do we need to run code we don’t use?
we don’t use 
Dispatchers.setMain(..)
anywhere because we inject our dispatchers
because it’s a best practice
r
Its simple , the main dispatcher is tied to the Android Framework. During Unit test that dispatcher is none existent therefore, we should set it to the test specific dispatcher
this explanation is documented in the kotlin coroutines docs. which means people who code behind you only need to look at docs to know what going on
z
We choose not to use 
Dispatchers.Main
for performance reasons for our 
ViewModel
s. So this would mean that every 
viewModelScope.launch { }
would be incorrect because it would need to be 
viewModelScope.launch(Dispatchers.Default) { .. }
every time. and to guarantee consistency there, that would mean to write a lint rule that would be unnecessary
r
How much of a true performance gain do you believe you get from that? have you done benchmarks?
z
referring to 
dispatchers.main
vs 
Dispatchers.Main
is straight forward
Why run code on the main thread that does not need to be run on the main thread?
r
if your interacting with UI components which most times you are than you need dispatchers.main. if not you can always switch dispatchers
z
the `ViewModel`’s 
State
, exposed as 
Flow<State>
, does not require the UI thread to build it fully
r
I mean i see what your trying to accomplish dont get me wrong. But I think there isn't much benefit espeically compared to the cost. You can switch dispatchers whenever you want
its the same concept in RXJava
ObserveOn(AndroidSchdulers.MAIN)
z
the only cost is that instead of using the Jetpack ViewModel factory, you use a different one with effectively the same API, just an additional parameter
Yes, in RxJava you should never directly refer to 
AndroidSchedulers.MAIN
outside of the place you construct your dispatchers
r
so are you saying you never create a class that extends ViewModel?
z
I create 1
Copy code
internal class ViewModelHolder<T>(
    private val coroutineScope: CoroutineScope,
    val viewModel: T
) : AndroidXViewModel() {

    override fun onCleared() {
        coroutineScope.cancel()
    }
}
r
yep boilerplate lol
z
?
no
r
def
and your cancling the scope. what your doing under the hood, is what viewModelScope does
its litterally a extract abstraction over whats already there
z
correct, but it allows you to inject your dependencies
it’s also completely abstracted from the rest of the codebase
so it was a one time cost in a large codebase to fix a poor API provided by AndroidX, and has greatly reduced test complexity for the team
r
I wouldn't say the API was poor. if it was poor than why make it the base lol. The thing is viewModel and viewModelScope do the exact same thing already
z
by making assumptions and relying on static state to achieve it
r
lol
its the same thing, trust me. your tieing the CoroutineScope lifecycle to the same static lifecycle that you hate
the benefit is trival at best
atleast in my opinion
i cant see the benefit
Both ViewModels are already testable and ViewModelScope, is just a CoroutineScope under the hood
z
ViewModel
is nothing but a scoping mechanism. You can take that scoping mechanism and add additional responsibility to it, like building states for a view to consume. Of course, while minor, this violates single responsibility
I need that scoping mechanism of course
Both ViewModels are already testable and ViewModelScope
Easy to test and difficult to test are two different things
i cant see the benefit
You want to have some `ViewModel`s do background work in 
Dispatchers.Default
. While others have a requirement to use 
Dispatchers.Main
. How do you achieve that with 
.viewModelScope
in a non-error prone way?
r
one sec Zak, brb
👍 1
@zak.taccardi my bad , got caught up in some other things . ViewModel is pretty much a service locator. The complexity is only at creation time , and thats only in the perspective of a activity or fragment. So if you said it adds complexity to dagger, I would 100% agree with you . Creation can be complicated, even tho dagger has been improved to handle the situtation. But as for testing, you test it the same way you would test any class
Viewmodelscope, is just a coroutineScope that is already created for you and cleared for you in Viewmodel.onClear.... There is no more complexity pass that. So the complexity isn't in testing viewModelScope, or Google implementation of it. Its just the complexity of testing coroutines in general . which takes abit of understanding kotlin coroutines to get it right
Constructor injection is a viable solution for handling Coroutine-scope, after all there is no easier way to substitute a dependency during testing. So if ts thats your approach by all means. But there are also other ways wich arent complicated as well. as for abstracting out jetpack viewModel to be a none jetpack viewmodel...i thing that part still sound a bit overkill
z
Viewmodelscope, is just a coroutineScope that is already created for you and cleared for you in Viewmodel.onClear.... There is no more complexity pass that.
this is becomes very complex if you need to: 1. change which dispatcher is used - in prod: 
Dispatchers.Main
vs 
Dispatchers.Default
- in test: 
TestCoroutineDispatcher
2. Cancellation semantics: - in prod: 
ViewModel.onCleared(..)
- in test: when 
@Test fun testFunction()
completes
I’m specifically pushing complexity in creation time, using a very similar API to what AndroidX already provides. So to other devs, there’s no real additional complexity. Only complexity is the code I had to write to provide this improved, more flexible API, which was a one time cost
825 Views
Open in Slack
PreviousNext
Powered by