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

ViewModel scope handling in 3.5.4-RC1 #1821

Closed
sixtsense opened this issue Mar 13, 2024 · 7 comments
Closed

ViewModel scope handling in 3.5.4-RC1 #1821

sixtsense opened this issue Mar 13, 2024 · 7 comments
Milestone

Comments

@sixtsense
Copy link

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() {
  ....
}
  1. Try to instantiate ExampleViewModel
val exampleScope = getKoin().getOrCreateScope<ExampleQualifier>(ExampleId)
val exampleViewModel = koinViewModel<ExampleViewModel> (
     viewModelStoreOwner = it,
     scope = exampleScope 
)
  1. 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
Copy link

Borislav-Nikolov commented Mar 14, 2024

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
Copy link

smiLLe commented Mar 18, 2024

Does not work with ScopeViewModel either, for me

@arnaudgiuliani
Copy link
Member

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.

@arnaudgiuliani arnaudgiuliani added the question Usage question label Mar 28, 2024
@Borislav-Nikolov
Copy link

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
Copy link
Author

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
Copy link
Member

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 arnaudgiuliani added type:issue and removed question Usage question labels Mar 29, 2024
@arnaudgiuliani
Copy link
Member

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

@arnaudgiuliani arnaudgiuliani added this to the 3.5.4-RC milestone Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants