InsertKoinIO / koin

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

Crash during Fragment replace for scoped ViewModel #492

Closed egorikftp closed 4 years ago

egorikftp commented 5 years ago

Describe the bug I was found an issue when I try to open fragment and then replace with the same fragment. In our prod application, it's a popular case.

java.lang.IllegalStateException: Definition without any InstanceContext - [type:Factory,scope:'com.egoriku.testapplication.fragment.CrashFragment', primary_type:'com.egoriku.testapplication.viewmodel.TestViewModel'] at org.koin.core.definition.BeanDefinition.resolveInstance(BeanDefinition.kt:71) at org.koin.core.scope.Scope.resolveInstance(Scope.kt:165) at org.koin.core.scope.Scope.get(Scope.kt:128) at org.koin.androidx.viewmodel.ViewModelResolutionKt$createViewModelProvider$1.create(ViewModelResolution.kt:66) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:135) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:103) at org.koin.androidx.viewmodel.ViewModelResolutionKt.getInstance(ViewModelResolution.kt:43) at org.koin.androidx.viewmodel.ViewModelScopeResolutionKt.getViewModel(ViewModelScopeResolution.kt:14) at org.koin.androidx.viewmodel.ext.android.ScopeExtKt.getViewModel(ScopeExt.kt:86) at com.egoriku.testapplication.fragment.CrashFragment$$special$$inlined$viewModel$1.invoke(ScopeExt.kt:96) at com.egoriku.testapplication.fragment.CrashFragment$$special$$inlined$viewModel$1.invoke(ScopeExt.kt) at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74) at com.egoriku.testapplication.fragment.CrashFragment.getModel(CrashFragment.kt) at com.egoriku.testapplication.fragment.CrashFragment.onViewCreated(CrashFragment.kt:26)

To Reproduce I have prepared a sample demo project with a crash.

https://github.com/egorikftp/KoinBug

Steps to reproduce the behaviour:

  1. Install the demo app
  2. Click on 'Replace fragment'
  3. Then click to button once again
  4. See error

Expected behaviour New fragment instance created without a crash.

Koin project used and used version: koin version 2.0.1

Note There is no crash in case I use add fragment in the sample.

MehulKK commented 5 years ago

Getting same error while replace fragment with BottomNavigationView. https://stackoverflow.com/questions/56697564/app-crash-once-replace-fragment-for-scoped-viewmodel

ShwetaChauhan18 commented 5 years ago

@egorikftp: Can you please try below code. I checked your GitHub code and tried.

  1. In your module

    val myModule = module {
    scope(named("CrashFragment")) {
        scoped<IService> { MyService() }
    
        viewModel {
            TestViewModel(get())
        }
    }
    }
  2. In CrashFragment private val viewModelScope = getKoin().getOrCreateScope("IService",named("CrashFragment")) private val model: TestViewModel = viewModelScope.get()

  3. In CrashFrgament onDestroy() method override fun onDestroy() { super.onDestroy() viewModelScope.close() }

MehulKK commented 5 years ago

@ShwetaChauhan18 : thanks solve this issue.

@egorikftp : you can apply this solution. its solve at my end.

egorikftp commented 5 years ago

@ShwetaChauhan18 Thanks for your solution. I see that it works on the sample project, but the solution looks strange)

I'll wait official fix.

ljubisa987 commented 5 years ago

Looks like issue occurs due to BeanDefinition being closed, through closing scope of previous fragment, hence instance is nullified and unable to be reused in any following fragment with the same name. Same may happen for activities actually, but less prominent due to different lifecycle.

This makes use of currentScope feature impossible for cases when fragments needs to be replaced or added/removed, and we run into multiple issues in our project due to this.

@arnaudgiuliani Maybe BeanRegister should avoid clearing definitions that are used in some other scopes? Would you be kind to share the progress on the fix?

arnaudgiuliani commented 5 years ago

can you try again with 2.1.0-alpha-6?

egorikftp commented 5 years ago

Yep, I will try soon and write about results. Thanks

TooLazyy commented 4 years ago

that's not a good idea to use viewModelScope.get() to get viewmodel, we should use viewModelScope.viewModel() for that. But it doesnt help in my case. Only up to version 2.1.0-alpha-7 helped.

egorikftp commented 4 years ago

@arnaudgiuliani Looks like issue have fixed.

Also, I have found another bug with scoped dependencies. I will open another issue. Thank you so much.