Breaking Changes for scoped view model in version 3.5.3

[Ucash]

Describe the bug Using activity scope, the koinNavViewModel method creates a new view model each time if the default key and qualifier arguments are used. As a workaround I had to add a key argument.

Initial investigation I have found that org.koin.androidx.viewmodel.resolveViewModel method internally use new getViewModelKey method which builds view model key based on provided key, qualifier and id of a scope if it is not the root one. In the result when key and qualifier are left default (both are nulls) the same key is generated for each view model and ViewModelProvider calls factory each time view model type is not matched.

Is this intentional change that from 3.5.3 for scoped view model key or qualifier argument is required?

[will-nolan]

We're also seeing the same behaviour. Scoped viewmodels injected via Koin are now giving multiple instances whereas previously the same instance across the scope was expected.

[dees91]

I observe similar erroneous behavior:

scope<ParentActivity> {
        factory { SomeDependency(get()) } 
        viewModelOf(::FragmentXViewModel)
        viewModelOf(::FragmentYViewModel)
}

In 3.5.0, FragmentXViewModel and FragmentYViewModel received the same SomeDependency instance within the ParentActivity scope - for example, in the situation of a transition from fragment X to fragment Y within the same ParentActivity launch. In 3.5.3 they get different instances of SomeDependency in the described situation.

[congvc-dev]

@dees91 I think v3.5.3 behavior is least astonishing because factory { } will provide a new SomeDependency instance each time a viewModel requests it

[dees91]

@congvc-apero You are right that in this simplified snippet factory works as you described, but I did not include important details about acitivity/fragment scope settings, that's why I prepared a repo with example that view models get the same instance of SomeDependency within the ParentActivity scope in 3.5.0 and they get a different instance in 3.5.3: https://github.com/dees91/koinexperiment/tree/main/app/src/main/java/pl/deesoft/koinexperiment/experiment

3.5.0	3.5.3

[congvc-dev]

@dees91 pardon me if I understand your usecase wrong. So basically, you want to share the same SomeDependency instance to all ViewModel instances correct? I tried ScopeSDL.scoped { } dsl (in 3.5.3) and your code works fine, as it should. Indeed, factory { } broke your code, but in a good way; it works as it should.

For clarify, here is CL I made

-    factory { SomeDependency() }
+    scoped { SomeDependency() }

[dees91]

@congvc-apero Thank you for your reply.

So basically, you want to share the same SomeDependency instance to all ViewModel instances correct?

Within the scope of ParentActivity, yes. Exiting and re-entering ParentActivity creates a new instance of SomeDependency. The scoped dsl may be applicable in my case.

However, referring to

broke your code, but in a good way

I would like to determine:

• whether the change in factory behavior in this case is an intentional fix for erroneous Koin behavior in previous versions (factory in this exact case returned the same instance for sure in Koin versions 3.4.3 and 3.5.0 and probably in earlier versions as well)
• or the change in the behavior of factory in this case in version 3.5.3 is an accidental regression

This is important because I would like to know whether the behavior from previous versions in this situation will return in Koin 3.5.4 or this is the expected behavior.

[volkert-fastned]

Interesting how there are multiple behavioral/breaking changes going from 3.5.1 to 3.5.3. See also this issue I reported last month: https://github.com/InsertKoinIO/koin/issues/1738

A patch-level version update should not have breaking changes.

[congvc-dev]

@dees91 in that case, I'd like to keep track of the issue and see how it will be resolved.

[arnaudgiuliani]

I would like to determine: whether the change in factory behavior in this case is an intentional fix for erroneous Koin behavior in previous versions (factory in this exact case returned the same instance for sure in Koin versions 3.4.3 and 3.5.0 and probably in earlier versions as well) or the change in the behavior of factory in this case in version 3.5.3 is an accidental regression

Here it's more a regression due to VM key build refactoring. IF I have proper tests/examples to reproduce to add to examples app, I can quickly patch it. Sorry for the break 🙏

[arnaudgiuliani]

Fixed. See PR. Let's check 3.5.4-RC1