Binary breaking changes in 2.2.0

[drewhamilton]

Describe the bug
In Koin 2.2.0, some extension functions related to Scope were removed. This causes a binary incompatibility.

To Reproduce

1. Compile a library with Koin 2.1 which internally uses one of the removed functions, like fun <T : Any> T.getScopeId(): String.
2. Create an app that consumes the library, and also consumes Koin 2.1. Set up the app such that the library's internal use of getScopeId is hit at runtime.
3. Bump up the app to Koin 2.2.

Expected behavior
The app compiles and runs as before.

Actual behavior
The app compiles, but crashes at runtime with a NoSuchMethodError.

This presents a problem for my team, which ships many libraries which use several such methods (particularly the one in the example, as well as a Scope.viewModel extension). Our consumers want to update Koin, but can't because they get this crash. And we can't gradually allow support; we would need to update all of our libraries to Koin 2.2.0 at the same time.

I propose to re-add the removed functions with @Deprecated markings in a 2.2.x release. They can even be given the ERROR or HIDDEN levels if you want to strongly discourage their use, but we need the runtime compatibility.

I would be happy to open a PR for this if it can be shipped in a 2.2.x release.

Koin project used and used version
koin-android-viewmodel version 2.2.0

[arnaudgiuliani]

All those extensions are unlockable with KoinScopeComponent. On one side I agree this is a hard breaking. On the other side we need to avoid pollute API, with extensiosn that you will be in just some cases.

did you tried KoinScopeComponent? https://doc.insert-koin.io/#/koin-core/scopes?id=scope-component-associate-a-scope-to-a-component-220

[drewhamilton]

KoinScopeComponent is only introduced in 2.2.0, the same version where the old functions are removed: there's no overlap where we can migrate from the old functions to the new way.

It does look like KoinScopeComponent would work for us, but the need to all-at-once migrate many libraries to 2.2.0 is the problem.

I think having the functions deprecated (or even hidden) lets you move toward un-polluting the API while also allowing a transition period for consumers.

[drewhamilton]

Any way I can convince you to accept a PR re-adding these deprecated functions? I can handle re-adding the removed functions with ERROR-level deprecations and ReplaceWiths provided where possible. I wouldn't need this maintained long term (e.g. the 3.x line can leave them removed); I just need at least one "overlap" version that we can use while our various teams migrate to the new APIs.

[fpr0001]

I also have this problem. @drewhamilton proposal seems like a win-win situation, since the functions would be deprecated (and even hidden). This would solve a huge headache for our multi-library code base

[arnaudgiuliani]

Any way I can convince you to accept a PR re-adding these deprecated functions? I can handle re-adding the removed functions with ERROR-level deprecations and ReplaceWiths provided where possible. I wouldn't need this maintained long term (e.g. the 3.x line can leave them removed); I just need at least one "overlap" version that we can use while our various teams migrate to the new APIs.

Yep, let's check for a proposal. Master is already migrated for 3.0.1. I'm still wondering if I ask people to go from 2.x to new 3.0.1

[drewhamilton]

OK, do you have a standard format you'd like me to use for a proposal?

Otherwise I can just describe the approach I had in mind:

1. Do an API diff between the 2.1.6 and 2.2.2 tags. (I'd use Kotlin/binary-compatibility-validator; up to you if you want to keep that in the final PR or not.)
2. Branch off of the 2.2.2 tag.
3. Re-add all broken APIs with a deprecation level. Prefer WARNING or ERROR so that ReplaceWith can provide automatic migrations to the new APIs, but HIDDEN may make sense in some cases.
4. Release 2.2.3 from that branch.

For example, in org.koin.android.viewmodel.scope.ScopeExt.kt I would add the following code:

//region Deprecated
@Deprecated(
    "Replace with ViewModelOwnerDefinition-based overload",
    ReplaceWith(
        "viewModel(" +
                "qualifier = qualifier, " +
                "owner = { ViewModelOwner.from(owner) }, " +
                "mode = LazyThreadSafetyMode.NONE, " +
                "parameters = parameters" +
                ")",
        "org.koin.android.viewmodel.ViewModelOwner"
    ),
    DeprecationLevel.ERROR
)
inline fun <reified T : ViewModel> Scope.viewModel(
    owner: ViewModelStoreOwner,
    qualifier: Qualifier? = null,
    noinline parameters: ParametersDefinition? = null
): Lazy<T> {
    return lazy(LazyThreadSafetyMode.NONE) {
        getViewModel(
            ViewModelParameter(
                T::class,
                qualifier,
                parameters,
                owner.viewModelStore
            )
        )
    }
}

@Deprecated(
    "Replace with direct call to ViewModelParameter-based overload",
    ReplaceWith(
        "getViewModel(ViewModelParameter(T::class, qualifier, parameters, owner.viewModelStore))",
        "org.koin.android.viewmodel.ViewModelParameter"
    ),
    DeprecationLevel.ERROR
)
inline fun <reified T : ViewModel> Scope.getViewModel(
    owner: ViewModelStoreOwner,
    qualifier: Qualifier? = null,
    noinline parameters: ParametersDefinition? = null
): T {
    return getViewModel(ViewModelParameter(T::class, qualifier, parameters, owner.viewModelStore))
}

@Deprecated(
    "Replace with direct call to ViewModelParameter-based overload",
    ReplaceWith(
        "getViewModel(ViewModelParameter(clazz, qualifier, parameters, owner.viewModelStore))",
        "org.koin.android.viewmodel.ViewModelParameter"
    ),
    DeprecationLevel.ERROR
)
fun <T : ViewModel> Scope.getViewModel(
    owner: ViewModelStoreOwner,
    clazz: KClass<T>,
    qualifier: Qualifier? = null,
    parameters: ParametersDefinition? = null
): T {
    return getViewModel(
        ViewModelParameter(
            clazz,
            qualifier,
            parameters,
            owner.viewModelStore
        )
    )
}
//endregion

Whether you then want to merge this to 3.x/master is up to you. For me, breaking changes from 2.x to 3.x is more expected, so it's the migration from 2.1.x to 2.2.x that is the more important problem to fix.

[arnaudgiuliani]

let's continue there #996

[drewhamilton]

Hi, I saw your comment on #996 but then saw you reopened this issue, should we continue there or here?

[drewhamilton]

Hi @arnaudgiuliani, my consumers are asking about this again. You indicated some interest, but I was unclear on whether you are in favor of my proposed solution – namely, re-adding removed functions as "deprecated" in a Koin 2.2.x release. If you are in favor I am willing to do this work and open a PR for it.

To summarize my proposal:

• Removed APIs would be re-added as deprecated to 2.2.x, but not to 3.x.
• Nothing that is already in 2.2.x would be removed.

As a result, 2.2.x will become an "overlap" line, where consumers have the freedom to stop using deprecated APIs and start using KoinComponent etc.

[paristote]

Hi @arnaudgiuliani , any update on this?

[arnaudgiuliani]

2.0.1, 2.1.6 & 2.2.2 will be ported to Maven Central
All fixes will be backported from 3.0.x to 2.3.0

Target is Q2 (April/May)

[paristote]

@arnaudgiuliani thanks but it looks like you are talking about #1004 :)
Drew offered his help to create a PR that brings back the deleted functions as deprecated, only if you are ok with that. Are you?

[arnaudgiuliani]

Jcenter won't accept any new artifacts very soonly. There is a bit more than the broken APIs. There is also fixes with them :)

[arnaudgiuliani]

I will close the current issue, as everything should be better in 3.1.3. Please upgrade, as it's has been fixed in 3.1.x.

[fpr0001]

@arnaudgiuliani this typealias is gone in v3.1.3, so we still cannot upgrade. Can you put it back in v.3.1.4?

[arnaudgiuliani]

@fpr0001 yes, I can add it back in 3.1.4.

[arnaudgiuliani]

If you need more help, come here: #1212