evant / kotlin-inject

Dependency injection lib for kotlin
Apache License 2.0
1.29k stars 60 forks source link

Support multiple scopes for components #433

Open vRallev opened 2 months ago

vRallev commented 2 months ago

We'd like to rename and migrate some of our scope annotations, e.g. today we use @SingleInAppScope, but would like to change this to @SingleIn(AppScope::class). Since this change needs to be rolled out in multiple repositories, this has to be done incrementally.

Dagger 2 supports multiple scopes per component and something like that would be valid:

@SingleInAppScope
@SingleIn(AppScope::class)
@Component
interface MyComponent

When checking if all providers use the correct scope, Dagger basically treats both scopes the same. This is great, because it would make the migration a lot easier.

However, kotlin-inject doesn't support multiple scopes for a component and simply picks the first one: https://github.com/evant/kotlin-inject/blob/main/kotlin-inject-compiler/core/src/main/kotlin/me/tatarka/inject/compiler/InjectGenerator.kt#L199

Would you be open to support multiple scopes? Otherwise I don't see a way to migrate from one scope to another incrementally.

evant commented 2 months ago

and simply picks the first one

Yeah it's not great to silently drop it. It really should either error out or support multiple like you said. Migration seens a reasonable enough reason to and I don't really see this causing any confusion.

evant commented 2 months ago

Oh hm, actually there's a potential for confusion here if you are using a different scope on say the component and a parent interface. I believe there's already a check for conflicting scopes there. Would enhancing that to ensure they all match work? I think that would still satisfy the migration goal.

vRallev commented 2 months ago

What exactly do you mean, can you give me an example? I struggle to follow. For example, I'd consider this an error:

@ParentScope
@Component
abstract class ParentComponent

@ParentComponent
@ChildComponent
@Component
abstract class ChildComponent(
  @Component val parentComponent: ParentComponent,
)
evant commented 2 months ago

Right now this

interface SomeComponentInterface

@MyScope abstract class MyComponent : SomeComponentInterface

is equivalent to

@MyScope interface SomeComponentInterface

abstract class MyComponent : SomeComponentInterface

is equivalent to

@MyScope interface SomeComponentInterface

@MyScope abstract class MyComponent : SomeComponentInterface

and this errors

@MyScope interface SomeComponentInterface

@DifferentScope abstract class MyComponent : SomeComponentInterface

I want to ensure that error remains instead of treating the scopes the same because it's likely the user applied the wrong scope.

@MyScope @DifferentScope interface SomeComponentInterface

@MyScope @DifferentScope abstract class MyComponent : SomeComponentInterface

should be ok

vRallev commented 2 months ago

Ah, thanks for explaining. That makes sense.