Skip to content
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

Closed
AKJAW opened this issue Dec 22, 2022 · 9 comments
Closed

Comments

@AKJAW
Copy link

AKJAW commented Dec 22, 2022

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:

import com.rickclephas.kmm.viewmodel.stateIn
import kotlinx.coroutines.flow.stateIn as normalStateIn

class TestViewModel : KMMViewModel() {

    val intViewModelScopeFlow: StateFlow<Int> = createFlow()
        .stateIn(viewModelScope, SharingStarted.WhileSubscribed(), 0)

    val intCoroutinesCopeFlow: StateFlow<Int> = createFlow()
        .normalStateIn(viewModelScope.coroutineScope, SharingStarted.WhileSubscribed(), 0)

    private fun createFlow() = flow {
        var i = 1
        while (true) {
            delay(1000)
            emit(i++)
        }
    }
}

From the source code it seems that com.rickclephas.kmm.viewmodel.stateIn uses the equivalent of what I've used

/**
 * @see kotlinx.coroutines.flow.stateIn
 */
@Suppress("NOTHING_TO_INLINE")
public actual inline fun <T> Flow<T>.stateIn(
    viewModelScope: ViewModelScope,
    started: SharingStarted,
    initialValue: T
): StateFlow<T> = stateIn(viewModelScope.coroutineScope, started, initialValue)

iOS

But the StateFlow which uses the normalStateIn is not correctly updated on iOS:

struct TaskListScreen: View {
    
    @StateViewModel var testViewModel = TestViewModel()
    
    var body: some View {
        NavigationView {
            VStack {
                Text("ViewModelScope \(testViewModel.intViewModelScopeFlowNativeValue)")
                Text("CoroutinesScope \(testViewModel.intCoroutinesCopeFlowNativeValue)")
            }

        }
    }
}

Screenshot 2022-12-22 at 20 35 04

Android

On Android both of the StateFlows behave in the same way:

Column {
    val viewModel = viewModel<TestViewModel>()
    val vm = viewModel.intViewModelScopeFlow.collectAsState().value
    Text("ViewModelScope $vm")
    val normal = viewModel.intCoroutinesCopeFlow.collectAsState().value
    Text("CoroutinesScope $normal")
}

Screenshot 2022-12-22 at 20 30 36

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

@joreilly
Copy link

joreilly commented Dec 22, 2022

I believe issue is that you're missing @NativeCoroutinesState ....have example of it's use in following fwiw

https://github.com/joreilly/Confetti/blob/main/shared/src/commonMain/kotlin/dev/johnoreilly/confetti/ConfettiViewModel.kt#L19

@rickclephas
Copy link
Owner

Thanks for the detailed feedback! The result you are seeing is excepted.
On Android the KMM-ViewModel stateIn function is indeed an "alias" for the Kotlin Coroutines stateIn function.
However on iOS it uses a custom implementation: https://github.com/rickclephas/KMM-ViewModel/blob/091dd2a4fbe390e6f73a5b963f11996e15317641/kmm-viewmodel-core/src/appleMain/kotlin/com/rickclephas/kmm/viewmodel/StateFlow.kt#L91-L102

The "magic" is all in the ViewModelScope which forwards state changes to your Swift code.

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.

That is correct. I have tried to make the dependency on the library as minimal as possible.
Adding (or removing) the library is a matter of adding a couple of imports and in some places the ViewModelScope parameter.

It will also make testing code harder because as far as I see Faking ViewModelScope might not be so easy (I might be wrong).

True, the ViewModelScope isn't easy to fake at the moment. Could you elaborate on what you are missing in terms of testability? Are you looking to use a custom CoroutineScope or ViewModelScope (which contains the CoroutineScope)?

Still, I think it's better for the library API surface be limited to only the ViewModel and allow the flexibility of CoroutineScope.

That would indeed be great, however we need some way to tell Swift that the state has changed, which is only known by the StateFlows.

@rickclephas
Copy link
Owner

rickclephas commented Dec 22, 2022

FYI the @NativeCoroutinesState annotation (from KMP-NativeCoroutines) @joreilly mentioned will improve the property names in your Swift code (e.g. intViewModelScopeFlow instead of intViewModelScopeFlowNativeValue).
But without the use of the KMM-ViewModel stateIn function it won't help to propagate state changes to Swift.

@joreilly
Copy link

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)

@AKJAW
Copy link
Author

AKJAW commented Dec 22, 2022

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.
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

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.
Having a TestViewModel base class / implementation class that allows passing dispatcher would also pollute the client code if used in production. But in case of passing the ViewModelScope it could help (when the VM needs to be created in test code).

Launching coroutines is fine, because you can use just use the coroutine scope:

viewModelScope.coroutineScope.launch(dispatcherProvider.io)

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

@rickclephas
Copy link
Owner

Alright, so it would be possible to add a way to pass a custom Dispatcher, however that wouldn't be compatible with the Android viewModelScope (which can't be customised).
Dispatchers.setMain is actually the recommended way to replace the dispatcher in unit tests.

How would you use the dispatcherProvider if the view model was just a Jetpack ViewModel?

To reduce the dependency on KMM-ViewModel, I wouldn't use the ViewModelScope outside the KMMViewModel.
Instead I would forward the CoroutineScope and use the stateIn function in the view model.

@AKJAW
Copy link
Author

AKJAW commented Dec 23, 2022

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.

.stateIn(viewModelScope + Dispatchers.io, ...)

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

@rickclephas
Copy link
Owner

Ah yes of course that makes a lot of sense. Will add some more stateIn overloads for that.

Regarding the setMain boilerplate, I guess you could hide it in a custom runTest:

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()
    }
}

@rickclephas
Copy link
Owner

FYI in v1.0.0-ALPHA-3 there is a new stateIn overload that accepts a CoroutineContext.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants