https://kotlinlang.org
Join Slack
A bit coroutine-y a bit Android-y, but does using `viewModelScope` seem bad? Like isn't that somethi...
# coroutines
c
A bit coroutine-y a bit Android-y, but does using 
viewModelScope
seem bad? Like isn't that something that should be injected? But then again, like i'm not smart enough to know what the alternative would be lol
c
Nope, you should be using 
viewModelScope
. The whole idea is that you only want stuff running as long as the ViewModel is active, but the Android framework is what determines how long a ViewModel remains active. If you’re injecting the scope yourself, then you’d responsible for determining when to cancel those coroutineScopes, which you probably don’t want to do. Remember that a ViewModel typically lives longer than the Activity/Fragment it’s associated with, and it’s difficult to properly scope the lifetime of a ViewModel to the Android system lifecycle, which is why it’s best to just let the Android team figure out those details For things that are intended to live longer than a ViewModel (for example, Singletons, Repositories, etc.) those probably should be managed on your own CoroutineScope where you control their lifetime, rather than using an Android ViewModel and its 
viewModelScope
k
^ disagree. 
viewmodelScope
has a hard dependency on 
Dispatchers.Main.immediate
. If you use 
viewModelScope
it will require you to use the horrible 
Dispatchers.setMain
and 
Dispatchers.resetMain
APIs. Recently the arch libs allow for a vararg 
Closeable
to be injected into their constructor so you should be injecting a 
CoroutineScope
that can be closed to your viewmodels and using that. Then in your tests you can control what scope is passed in and avoid needing to use 
viewModelScope
as well as the 
Dispatchers.setPain
apis. In an upcoming release the 
viewModelScope
extension is actually going to be annotated with the 
@Discouraged
annotation for this very reason.
c
I was not aware of that recent change, that’s a really good call-out. @kevin.cianfarini can you paste a link to that change, I’m not able to find it with a quick Google search But yes, definitely prefer injecting the 
CoroutineScope
with 
Closable
in this case, as you still do not want to be trying to manage that lifecycle yourself.
c
@kevin.cianfarini thanks for the outline, but (i hate to admit it) that I don't know where that leaves me. It seems like right now, I use hilt to construct my VMs for me, but if I want to go this other route then I need to construct VMs "by hand"?
k
@Colton Idle I don’t know what fancy things you can do with hilt to help you. But here’s how I would do it with vanilla Dagger.
Copy code
class CloseableCoroutineScope(override val coroutineContext) : CoroutineScope, Closeable {
  override fun close() = cancel()
}

class MyFancyViewModel @Inject constructor(scope: CloseableCoroutineScope) : ViewModel(scope)

// In Dagger

@Provides fun providesCloseableCoroutineScope() = CloseableCoroutineScope(SupervisorJob())
Notice that the provided scope is not a singleton. A new scope will be created every time that function is called.
c
Cool. And one more question (since I unfortunately dont test my VMs too often) but you said
If you use 
viewModelScope
it will require you to use the horrible 
Dispatchers.setMain
and 
Dispatchers.resetMain
APIs.
you mean
If you use 
viewModelScope
it will require you to use the horrible 
Dispatchers.setMain
and 
Dispatchers.resetMain
APIs while writing tests.
Right?
k
Yes that’s correct
c
ahhh
im getting it Thank you all for teaching
k
No problem 🙂
There really should be an article out there which goes over this stuff called “Please, just inject a fucking scope” 😂
c
Idk https://developer.squareup.com/blog/please-just-inject-a-fucking-scope has a nice ring to it.
Authored by Kevin Cianfarini
k
Ah I don’t work at Square anymore but I’ll tell some friends from there to use the title
c
Its nice to know that my hunch was right. I think I will wait until hilt is updated with some of this new stuff. my team currently uses hilt + AAC VM + AAC nav and we just get so many free things from that (and so we're so entrenched in it) that itd be hard to move away.
k
You don’t have to drop hilt or AAC VM to stop using 
viewModelScope
There’s nothing to lose here
c
ooh. I guess that's true? wait. lemme try that dagger code out that you wrote here.
r
Might just be me but the idea of injecting a stateful object which the created VM takes ownership of seems a bit off for my conceptions of how object creation should work
Would suggest tweaking the approach so the injected object is a Factory that provides closeable scopes which are created on demand by the VM 🤔
But hoping Google will provide some opinionated advice if they’re discouraging the currently adopted pattern
k
You can always make the scope you pass in the parent of the scope you end up using. Eg.
Copy code
class ViewModelThing @Inject constructor(
  parentScope: CloseableCoroutineScope,
) : ViewModel(CloseableCoroutineScope(SupervisorJob() + scope)) { ... }
f
I recently refactored a sample project to replace 
viewmodelScope
with a closeable scope, you could have a look for reference https://github.com/fvilarino/Weather-Sample/blob/feature/compose/presentation/shar[…]a/com/francescsoftware/weathersample/shared/mvi/MviViewModel.kt
the scope is provided to viewmodels via injection using Hilt
k
So sad to hear they want to replace 
viewModelScope
, just with some different configuration, so we could have just a bit more ownership of it. If you want to have ownership of your 
CoroutineScope
, you can have it already.
f
The issue with 
viewmodelScope
is the implicit dependency on the main dispatcher
k
For tests, it's easy to handle it with few lines of code, or actual concrete implementation, you can just run it on different dispatcher.
We all know that they've baked that scope into viewmodel, and it was not the best choice, but choice was made, and it's core of many apps.
f
Nobody is forcing you to change anything, if you want to use viewModelScope you can go ahead and use it. If you want to have control on the context the coroutines run on, which is important for tests, then you have a better option as described in this thread
k
The small amount of convenience you get from VM scope does not even come close to the amount of testing burden it imposes
Injecting a scope to your VMs is literally one line of code
k
Can't you inject it now (dispatcher), and have your own dispatcher as 
viewModelScope.launch(myDispatcher)
?
k
You would still lack the ability to pass in a controlled scope that does two things: 1. the in case of runTest you wouldn't be able to pass in a scope that can control virtual time (thus making delay and debounce calls untestable). This could be solved by passing in the test scheduler provided by a test scope, but it seems excessive. 2. The test scope will report errors at the end of the test. Since viewModelScope is separate from the parent coroutine scope running the test you break structured concurrency and will potentially drop errors without realizing it.
Also good luck enforcing that every launch, async, flow, etc etc all run in a provided context/dispatcher so you can test
k
the in case of runTest you wouldn't be able to pass in a scope that can control virtual time (thus making delay and debounce calls untestable). This could be solved by passing in the test scheduler provided by a test scope, but it seems excessive.
I've never tried it, but I'd expect for one of 
TestDispatchers
to give you more control over calls in tests.
Also good luck enforcing that every launch, async, flow, etc etc all run in a provided context/dispatcher so you can test
Well, I'm not saying it's perfect, I'm just saying that you can still have benefits of 
viewModelScope
and inject 
Dispatcher
over constructor for tests. I thought this served me well in the past.
k
You should be doing both of those things and injecting 
EmptyCoroutineContext
for dispatchers under test
but a big issue is still testing if something errors. I use 
assertFailsWith
a lot and if viewmodelScope is dropping those errors on the ground because it has no parent scope to report to then you've got a problem.
p
instead of
Copy code
class ViewModelThing @Inject constructor(
  scope: CloseableCoroutineScope,
) : ViewModel(scope) { ... }
you can also just
Copy code
class ViewModelThing @Inject constructor(
  scope: CoroutineScope,
) : ViewModel(scope.asCloseable()) { ... }

fun CoroutineScope.asCloseable() = Closeable { cancel() }
if you don’t want to create a custom 
CloseableCoroutineScope
interface/class and provide it in your DI graph
k
I think that's fine, but it would require two allocations for one semantic thing.
m
@kevin.cianfarini
You should be doing both of those things and injecting 
EmptyCoroutineContext
for dispatchers under test (edited)
you mean both 
CloseableCoroutineScope
should be passed to 
ViewModel
and custom dispatchers should be injected to do proper testing?
62 Views
Open in Slack
PreviousNext
Powered by

kotlinlang

A modern programming language that makes developers happier.

Powered by