-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using stateIn with viewModelScope.coroutineScope does not update on iOS #6
Comments
I believe issue is that you're missing |
Thanks for the detailed feedback! The result you are seeing is excepted. The "magic" is all in the
That is correct. I have tried to make the dependency on the library as minimal as possible.
True, the
That would indeed be great, however we need some way to tell Swift that the state has changed, which is only known by the |
FYI the |
Ah, remember now....I was mixing this up with other issue I had getting iOS update (for that different reason due to view model wrapping in Swift code) |
Thanks for the fast feedback! I've added an edit for testing but it might be missed, so let me post it here: Played around with tests using the ViewModel scope, and it is not a pleasant experience :D You're forced to use Dispathers.setMain otherwise the test will always fail because we cannot change the dispatcher without using viewModel.coroutineScope + Dispatcher, however this change in production code will break iOS. Additional thing is that if we need to inject the ViewModelScope, we need to create an object / class which inherits KmmViewModel and pass in the viewModelScope field. You cannot create your own implementation of ViewModelScope, because the coroutineScope extention function is tied to the implementation detail of ViewModelScopeImpl, and will throw an exception if a different ViewModelScope implementation is used I guess the easiest way to improve testing could be to maybe allow passing in a dispatcher in the KMMViewModel? however that pollutes library the production code as it will be mostly used for testing. Launching coroutines is fine, because you can use just use the coroutine scope:
The problem arises when we the stateIn Function, maybe adding an additional dispatcher parameter to it, could help wiith this? The default value would be the Main.Immediate or Main for iOS, but the client can override it if needed and the test can use a TestDispatcher |
Alright, so it would be possible to add a way to pass a custom How would you use the To reduce the dependency on KMM-ViewModel, I wouldn't use the |
I think if I wanted to change the android viewModelScope dispatcher in a stateIn function without Dispatchers.setMain. I would use the add operator e.g.
It's probably not something I would do often, but having the possibility to do so makes the library more flexible in my opinion. But as you said Dispatchers.setMain should do the job just fine if we only keep the KMM ViewModelScope and stateIn functions inside the ViewModel, too bad we don't have the JUnit extensions in KMM to hide the setMain and resetMain boilerplate |
Ah yes of course that makes a lot of sense. Will add some more Regarding the inline fun runTest(
context: CoroutineContext = EmptyCoroutineContext,
dispatchTimeoutMs: Long = DEFAULT_DISPATCH_TIMEOUT_MS,
testBody: suspend TestScope.() -> Unit
): TestResult {
Dispatchers.setMain(UnconfinedTestDispatcher())
try {
return runTest(context, dispatchTimeoutMs, testBody)
} finally {
Dispatchers.resetMain()
}
} |
FYI in v1.0.0-ALPHA-3 there is a new |
Hi, I've tried to add the KMM-ViewModel to my project, however I've faced one big issue. I've prepared a branch that presents this issue: AKJAW/Timi-Multiplatform@cb8888f
Here's the code for the ViewModel:
From the source code it seems that com.rickclephas.kmm.viewmodel.stateIn uses the equivalent of what I've used
iOS
But the StateFlow which uses the normalStateIn is not correctly updated on iOS:
Android
On Android both of the StateFlows behave in the same way:
Expected result
The expected result would be that both com.rickclephas.kmm.viewmodel.stateIn and kotlinx.coroutines.flow.stateIn behave the same on iOS, which is currently not the case. Having to pass KMMViewModel.viewModelScope around everytime a StateFlow is created is problematic, because code will be tied to this library even though it doesn't need to be. It will also make testing code harder because as far as I see Faking ViewModelScope might not be so easy (I might be wrong). Still, I think it's better for the library API surface be limited to only the ViewModel and allow the flexibility of CoroutineScope.
Edit:
Played around with tests using the ViewModel scope, and it is not a pleasant experience :D You're forced to use Dispathers.setMain otherwise the test will always fail because we cannot change the dispatcher without using viewModel.coroutineScope + Dispatcher, however this change in production code will break iOS.
AKJAW/Timi-Multiplatform@9cf040b#diff-52ecb25b704f2601f1256c922a3fc5b45569daaf8ea3b9d51434907cdbbe7967
Additional thing is that if we need to inject the ViewModelScope, we need to create an object / class which inherits KmmViewModel and pass in the viewModelScope field. You cannot create your own implementation of ViewModelScope, because the coroutineScope extention function is tied to the implementation detail of ViewModelScopeImpl, and will throw an exception if a different ViewModelScope implementation is used
AKJAW/Timi-Multiplatform@9cf040b#diff-7c5b828548356898f03b363ec2ede9591723cb5791cab7c7a761dae664dd3170
The text was updated successfully, but these errors were encountered: