InsertKoinIO / koin

Koin - a pragmatic lightweight dependency injection framework for Kotlin & Kotlin Multiplatform
https://insert-koin.io
Apache License 2.0
8.77k stars 695 forks source link

ViewModel scope handling in 3.5.4-RC1 #1821

Closed sixtsense closed 3 months ago

sixtsense commented 3 months ago

With the changes introduced in this pull request (aiming to fix the previously broken view model scoping issue) passing a Scope to the resolveViewModel(...) function has been deprecated and KoinViewModelFactory now uses koin.scopeRegistry.rootScope for every view model. This introduces breaking changes for projects which have multiple active Scopes (for example application scope / user session scope). With the new changes KoinViewModelFactory would look for dependencies directly inside the root scope (application scope in my case) and fail to instantiate view models with dependencies scoped as a part of a user session scope.

Steps to reproduce the behavior:

  1. Create a custom Scope

val exampleScope = Koin.getOrCreateScope(ExampleId, ExampleQualifier, ExampleSource)

  1. Scope one or more dependencies under exampleScope
    
    scope(qualifier = ExampleQualifier) {
    scopeSet(this)
    scopedOf(::ExampleDependencieOne)
    scopedOf(::ExampleDependencieTwo)
    viewModelOf(::ExampleViewModel)
    }

class ExampleViewModel( private val scopedDependencyOne: ExampleDependencieOne, private val scopedDependencyTwo : ExampleDependencieTwo ) : ViewModel() { .... }


3. Try to instantiate `ExampleViewModel`

val exampleScope = getKoin().getOrCreateScope(ExampleId) val exampleViewModel = koinViewModel ( viewModelStoreOwner = it, scope = exampleScope )


4. View model instantiation fails due to an inability to find `ExampleDependencieOne` and `ExampleDependencieTwo` in the root scope

**Koin module and version:**
 `koin-core:3.5.4-RC1`
Borislav-Nikolov commented 3 months ago

I also encountered this problem.

For scoped view model:

Caused by: org.koin.core.error.NoBeanDefFoundException: No definition found for type 'com.mypackage.MyViewModel'. 
Check your Modules configuration and add missing type and/or qualifier!
    at org.koin.core.scope.Scope.throwDefinitionNotFound(Scope.kt:301)
    at org.koin.core.scope.Scope.resolveValue(Scope.kt:271)
    at org.koin.core.scope.Scope.resolveInstance(Scope.kt:233)
    at org.koin.core.scope.Scope.get(Scope.kt:212)
    at org.koin.androidx.viewmodel.factory.KoinViewModelFactory.create(KoinViewModelFactory.kt:25)
    at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:184)
    at org.koin.androidx.viewmodel.GetViewModelKt.resolveViewModel(GetViewModel.kt:71)
    at org.koin.androidx.viewmodel.GetViewModelKt.resolveViewModel(GetViewModel.kt:39)

If I scope the view model to the root scope:

 * Instance creation error : could not create instance for '[Factory:'com.mypackage.MyViewModel']': 
 org.koin.core.error.NoBeanDefFoundException: 
No definition found for type 'com.mypackage.MyScopedDependency'. 
Check your Modules configuration and add missing type and/or qualifier!
    org.koin.core.scope.Scope.throwDefinitionNotFound(Scope.kt:301)
    org.koin.core.scope.Scope.resolveValue(Scope.kt:271)
    org.koin.core.scope.Scope.resolveInstance(Scope.kt:233)
    org.koin.core.scope.Scope.get(Scope.kt:212)

I see that there's this new ScopeViewModel, I'll try if it works for my case, but '3.5.4' is a patch version that should have fixed a bug that I solved temporarily just by adding a key for the view models. Now I have to make some non-trivial changes at 100+ places. Moreover, it doesn't seem right that my view models should depend on Koin and have to extend a Koin view model. What if I have an abstract view model and don't want all of them to be Scoped?

smiLLe commented 3 months ago

Does not work with ScopeViewModel either, for me

arnaudgiuliani commented 3 months ago

The use of ScopeViewModel is to help handlign scope around ViewModel itself.

module {
    viewModelOf(::MyScopeViewModel)
    scope<MyScopeViewModel> {
        scopedOf(::Session)
    }    
}

class MyScopeViewModel : ScopeViewModel() {

    // on onCleared, scope is closed

    // injected from current MyScopeViewModel's scope
    val session by scope.inject<Session>()

}

You need to have a scope with MyScopeViewModel, and then use scoped instances from it.

Borislav-Nikolov commented 3 months ago

Hello, @arnaudgiuliani , Thank you for your reply. I'll try to explain my case with an example and why I think your suggestion won't work for me.

Let's say I have a user session scope where I have scoped the current AccountEntry dependency. Then I have a bunch of view models that use AccountEntry. In order to get AccountEntry, the view models have to be scoped under the user session. With the new release candidate, this is not possible.

Scoping multiple user sessions to multiple view models won't work because the view models are part of a single user session. They depend on what it provides.

Also, handling dependency injection is not part of the view model's responsibilities and it could also prove difficult to integrate your ScopeViewModel with an already existing codebase. For example, I could have an abstract GeneralOperationsViewModel where half of the extending view models are out of the user session and I don't need them to extend ScopeViewModel.

sixtsense commented 3 months ago

Why was this labeled as a question when it clearly is a behavioral/breaking change for anyone using koin 3.5.1 or below. It literally breaks any and all usage of scoped dependencies inside a view model.

arnaudgiuliani commented 3 months ago

I reverted back the capacity to use ViewModel and scopes. ViewModelScope is just intended to inject dependency at the same scope level. It should be Ok now.

arnaudgiuliani commented 3 months ago

Let's reopen something for 3.5.4-RC2 if really something goes wrong