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

2.2.0 breaks backwards compatibility and adds clutter by enforcing @KoinApiExtension #939

Closed
holgerbrandl opened this issue Oct 31, 2020 · 28 comments
Assignees
Labels
core important 🔥 status:checking currently in analysis - discussion or need more detailed specs type:issue
Milestone

Comments

@holgerbrandl
Copy link

When migrating from v2.1.6 to 2.2rc3, classes that use koin do not need only implement KoinComponent as before, but are also now required to be annotated with @KoinApiExtension. The intent is not clear.

The required annotation makes it considerably more cumbersome to use koin (which worked just great previously) for library development, where users will sub-class core types that are built on top of KoinComponent.

As an example, see https://github.com/holgerbrandl/kalasim/blob/9cbbf4ce6d9dff063ae9a667b42908c59bc4cd65/src/test/kotlin/org/github/holgerbrandl/kalasim/examples/kalasim/Refuel.kt#L74 where the user now has to tag each custom type (e.g. GasStation) to be resolved via koin in the simulation with an extra @KoinApiExtension.

It would be great if koin could still support the main mode of operation which is implementing KoinComponent without an additional mandatory flag on all child-classes.

@arnaudgiuliani
Copy link
Member

-rc4 is fixing back it 👍

@arnaudgiuliani arnaudgiuliani added the question Usage question label Nov 2, 2020
@mtangoo
Copy link

mtangoo commented Nov 12, 2020

still required in released version

@DDihanov
Copy link

Still required - I will be monitoring this issue, will upgrade once it's fixed.

Furthermore the imports are borked, from import org.koin.core.KoinComponent to import org.koin.core.component.KoinComponent in the 2.2.0 version. This means I have to replace every single import in my code where KoinComponent is used AND add the @KoinApiExtensions annotation. So best just to wait for the fix or I would have to replace what I already replaced when the fix comes out.

@batadamnjanovic
Copy link

batadamnjanovic commented Nov 19, 2020

@DDihanov same here in 2.2.1.
I need to annotate all subclasses as well as the base class

@batadamnjanovic
Copy link

batadamnjanovic commented Nov 19, 2020

From: https://medium.com/koin-developers/whats-next-with-koin-2-2-3-0-releases-6c5464ae5e3d

"The other thing I’ve seen around is the misuse of KoinComponent interface to extend service locator API. Be aware that as a Koin framework user, you shouldn’t need to use this interface. KoinComponent interface must be used to extend Koin API or “bootstrap” component where we don’t have Koin APIs for that. In your app, use constructor style injection as much as possible.
For such sensible API functions, we introduce the @KoinApiExtension annotation to enforce users to opt-in such API."

Not sure what is "service locator API"?

In your app, use constructor style injection as much as possible.

Field injection is really what I need. If I introduce constructor injection in my case, I would need to add that field in constructor in all subclasses

@DDihanov
Copy link

From: https://medium.com/koin-developers/whats-next-with-koin-2-2-3-0-releases-6c5464ae5e3d

"The other thing I’ve seen around is the misuse of KoinComponent interface to extend service locator API. Be aware that as a Koin framework user, you shouldn’t need to use this interface. KoinComponent interface must be used to extend Koin API or “bootstrap” component where we don’t have Koin APIs for that. In your app, use constructor style injection as much as possible.
For such sensible API functions, we introduce the @KoinApiExtension annotation to enforce users to opt-in such API."

Not sure what is "service locator API"?

In your app, use constructor style injection as much as possible.

Field injection is really what I need. If I introduce constructor injection in my case, I would need to add that field in constructor in all subclasses

That's rich, given the documentation says to extend the interface: https://start.insert-koin.io/#/getting-started/koin-components

@batadamnjanovic
Copy link

@DDihanov Also, check this: https://doc.insert-koin.io/#/koin-core/koin-component

@ispbox
Copy link

ispbox commented Nov 21, 2020

still required in released version

It is possible to workaround this issue with

@OptIn(KoinApiExtension::class)

This will stop propagation of this requirement to descendant classes.
But it will also require compiler option

    kotlinOptions {
        freeCompilerArgs += "-Xopt-in=kotlin.RequiresOptIn" // Opt-in option for Koin annotation of KoinComponent.
    }

It was not convenient for me to fix the issue, though, I realize that there was some reasoning behind it in terms of performance and (in my case) loose coupling to Koin :).

@breucode
Copy link

From: https://medium.com/koin-developers/whats-next-with-koin-2-2-3-0-releases-6c5464ae5e3d
"The other thing I’ve seen around is the misuse of KoinComponent interface to extend service locator API. Be aware that as a Koin framework user, you shouldn’t need to use this interface. KoinComponent interface must be used to extend Koin API or “bootstrap” component where we don’t have Koin APIs for that. In your app, use constructor style injection as much as possible.
For such sensible API functions, we introduce the @KoinApiExtension annotation to enforce users to opt-in such API."
Not sure what is "service locator API"?

In your app, use constructor style injection as much as possible.

Field injection is really what I need. If I introduce constructor injection in my case, I would need to add that field in constructor in all subclasses

That's rich, given the documentation says to extend the interface: https://start.insert-koin.io/#/getting-started/koin-components

I am also currently unsure how to start my Application without using KoinComponent (https://github.com/breucode/imisu/blob/323275a75a53d67e60ef53a150ad3df403a6cd17/src/main/kotlin/de/breuco/imisu/Application.kt). I have to instantiate a class somewhere and call a function on it. This class however needs some dependencies.

@holgerbrandl
Copy link
Author

I've just tried again with 2.2.1 and it seems @KoinApiExtension is no longer mandatory (and neither is the odd compiler flag to disable it). Could you confirm? I've seen that @batadamnjanovic still faced the same issue with 2.1.1.

All imports still need to rewritten, but this is just typing work, whereas the forced annotation on sub-classes was/is a more conceptual limitation of the API.

@ispbox
Copy link

ispbox commented Dec 5, 2020

I've just tried again with 2.2.1 and it seems @KoinApiExtension is no longer mandatory (and neither is the odd compiler flag to disable it). Could you confirm?

Version 2.2.1 still has warning in Android Studio: "Used to extend current API with Koin API. Add @KoinApiExtension to class XXX".
The only way for me to get rid of this warning is to specify @OptIn(KoinApiExtension::class) attribute.

@mtangoo
Copy link

mtangoo commented Dec 5, 2020

@holgerbrandl v2.2.0 here. It is not mandatory but have a warning like @ispbox reported

@holgerbrandl
Copy link
Author

Indeed @mtangoo same warning here. But that's still far better compared to compile error in the initial build.

The warning is clearly still confusing/annoying, as it is not sufficient to annotate the base class with but the warning pops up on all child-classes as well. In my understanding, the annotation should at max enforce that the developer is aware of the risks (or simply does not care). But stating this once at the base-class should be sufficient imho.

@breucode
Copy link

I am also currently unsure how to start my Application without using KoinComponent (https://github.com/breucode/imisu/blob/323275a75a53d67e60ef53a150ad3df403a6cd17/src/main/kotlin/de/breuco/imisu/Application.kt). I have to instantiate a class somewhere and call a function on it. This class however needs some dependencies.

I found a solution to my specific problem, which leads to not using KoinComponent at all.
See: breucode/imisu@5a85ae6#diff-9e0ce0b08af05360b6797a67b954e95fd3811e8de85223d634a5c3c42545b634

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Jan 13, 2021

I found a solution to my specific problem, which leads to not using KoinComponent at all.

KoinComponent is a way to extend Koin API for any new runtime. In terms of app developers, you shouldn't really need it (apart rare case where you don't have access to inject()). Most of the time, it's a matter of making a in a module.

If you are a SDK/Lib maker, you can clearly use KoinComponent as you need to bring new features.

Koin modules => configuration
Koin Component => New API

if you heavily relay on KoinComponent, ask you the question if you really need it.

@DDihanov
Copy link

DDihanov commented Jan 13, 2021

I found a solution to my specific problem, which leads to not using KoinComponent at all.

KoinComponent is a way to extend Koin API for any new runtime. In terms of app developers, you shouldn't really need it (apart rare case where you don't have access to inject()). Most of the time, it's a matter of making a in a module.

If you are a SDK/Lib maker, you can clearly use KoinComponent as you need to bring new features.

Koin modules => configuration
Koin Component => New API

if you heavily relay on KoinComponent, ask you the question if you really need it.

So how do you suggest we inject View classes without extending KoinComponent then?

One of the reasons I use Koin instead of Dagger is exactly because of this, you can easily extend KoinComponent and inject Android classes(custom compound Views for example) directly(also avoiding huge RecyclerView Adapter constructors if the View is in the ViewHolder). Otherwise I have to provide the dependency with a module and then give it to the View programatically in the Fragment.

This sounds okay but the moment you have to do this for every custom view that needs injection and when some of them are within ViewHolders and what not, it becomes a pain(imagine having to provide every single dependency your custom View in the ViewHolder can have via the Adapter constructor and THEN passing in to the ViewHolder in the bind/create method and then passing it to your custom compound view). Or what else? We provide the RecyclerView Adapters via modules too so we can avoid KoinComponent?

So you can see your point is not exactly correct.

@laim2003
Copy link

laim2003 commented Jan 18, 2021

I get that you say KoinComponent makes sense for classes who definetely can't access (for whatever reason) let's say, androids Context class. But doesn't removing KoinComponent and switching to constructor level injection simply means that we're moving the use of get() and inject() out of the class itself to the place we're we are calling the instructor? This (in my opinion) just moves the problematic part somewhere else and I don't understand why you should necessarily do that.

I may recite the Dagger to Koin migration guide:

Tag Thermosiphon & CoffeeMaker classes with KoinComponent interface to lazy inject Heater into a property, instead of getting it from its constructor
https://android.jlelse.eu/the-thermosiphon-app-from-dagger-to-koin-step-by-step-a09af7f5b5b1

@jasonctoms
Copy link

Not just injecting views themselves, but injecting into custom views. If you have an app that makes use of custom views, KoinComponent is your only choice if you want to inject classes into there (for example, navigator classes). There is no possibility for constructor injection. The flexibility of KoinComponent was one of Koin's best features over Dagger, and adding annotations all over your code base to use a core feature is not nice.

@laim2003
Copy link

laim2003 commented Jan 26, 2021

Not just injecting views themselves, but injecting into custom views. If you have an app that makes use of custom views, KoinComponent is your only choice if you want to inject classes into there (for example, navigator classes). There is no possibility for constructor injection. The flexibility of KoinComponent was one of Koin's best features over Dagger, and adding annotations all over your code base to use a core feature is not nice.

Had the the same problem. Another example would be android services or WorkManager and other components that you don't instantiate yourself. Not using KoinComponent is like removing one of the main reasons to go with koin

@HerrVoennchen
Copy link

HerrVoennchen commented Jan 31, 2021

I have to agree with the guys. Injecting anywhere is (was?) one of the most useful features of koin and therefor a real alternative for DI in Spring.

On large scale applications (like rich APIs) constructor only initialization is not an option, since it becomes a mess of constructor args and it is bound to that instance which has passed in (which can matter regarding scalability, if the defintion is not a singleton).
Another issue is constructor initialization forces me to make interfaces and classes public. So now internal classes cannot make use of DI or have to be public.

So if KoinComponent isn't intended for that (and reserved for libs, which is fine), then a field level annotation (or similar without annotations) is needed to make constructor init optional.

Edit: My current workarround on this is create an interface alike KoinTest to almost avoid the warnings (only that 1 left) and use this instead of KoinComponent ok, then the compiler flag is needed... so no workaround

@mradzinski
Copy link

mradzinski commented Feb 5, 2021

I have to agree with everyone here. Yes, you can use compiler args to completely disable the need to even annotate KoinComponent with @OptIn, but that's not the actual point here. KoinComponent is a crucial part of Koin's ability to work not only on custom views or such things, but on deeper architecture layers where you're not interacting with an object that "naturally" supports service location (such as Activities or Fragments support on Android).

To be honest, I truly don't see the danger in using KoinComponent given that all it does is literally provide GlobalContext. Unless I'm missing something here, I could as well use GlobalContext.get().get() and I would be incurring in the same "danger" 🤷🏻‍♂️.

Fun fact: there's no such thing as an object that "naturally" supports service location. On Android for example, such "natural" objects I mentioned above inherit ComponentCallbacks which funny enough uses either KoinComponent or GlobalContext internally.

@arnaudgiuliani arnaudgiuliani added important 🔥 status:checking currently in analysis - discussion or need more detailed specs type:issue core and removed question Usage question labels Feb 8, 2021
@arnaudgiuliani
Copy link
Member

Hello all,

my point point about KoinComponent is not to avoid using it. It's just to warn you that KoinComponent help you inject where you need, but shouldn't replace module configuration.

but pushing to "optIn" is perhaps a bit strong.

I'm thinking of proposing a 2.2.3 that will come back on few strong breakings, like removing the optIn of KoinComponent 👍

@arnaudgiuliani arnaudgiuliani self-assigned this Feb 8, 2021
@mradzinski
Copy link

my point point about KoinComponent is not to avoid using it. It's just to warn you that KoinComponent help you inject where you need, but shouldn't replace module configuration.

I totally understand your point @arnaudgiuliani. I know what I'm about to say will be a bit controversial, but no matter how opinionated you try something to be, people will still find ways to miss-use it. You can try help them by creating an opinion with a plethora of tools, documentation, etc, but in the end there's still people who develop entire apps using just activities dumping the entirety of their code there, so...

I think documenting the intention of KoinComponent should suffice to inform people who care about their code enough to read some docs.

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Feb 10, 2021

I think documenting the intention of KoinComponent should suffice to inform people who care about their code enough to read some docs.

this is also one of the point I see here. Constraining too much about it, as it is a key component could be too bad.

There is few breakings around for 2.2.2. I will make a fix for that 👍

Documentation & other examples project could help also.

@ichenhe
Copy link

ichenhe commented Mar 3, 2021

still required in released version

It is possible to workaround this issue with

@OptIn(KoinApiExtension::class)

This will stop propagation of this requirement to descendant classes.
But it will also require compiler option

    kotlinOptions {
        freeCompilerArgs += "-Xopt-in=kotlin.RequiresOptIn" // Opt-in option for Koin annotation of KoinComponent.
    }

It was not convenient for me to fix the issue, though, I realize that there was some reasoning behind it in terms of performance and (in my case) loose coupling to Koin :).

Thanks to @ispbox , I have found a better workaround, just add

kotlinOptions {
  freeCompilerArgs += "-Xopt-in=org.koin.core.component.KoinApiExtension"
}

that's all you need to do, without @OptIn(KoinApiExtension::class).

@arnaudgiuliani
Copy link
Member

if you can wait for 2.3.0, I will remove OptIn from KoinComponent 👍
It's already deploying on Koin 3.0.1

@arnaudgiuliani
Copy link
Member

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.

@arnaudgiuliani
Copy link
Member

If you need more help, come here: #1212

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core important 🔥 status:checking currently in analysis - discussion or need more detailed specs type:issue
Projects
None yet
Development

No branches or pull requests