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

Support multiple scopes for components #433

Open
vRallev opened this issue Sep 9, 2024 · 5 comments
Open

Support multiple scopes for components #433

vRallev opened this issue Sep 9, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@vRallev
Copy link
Contributor

vRallev commented Sep 9, 2024

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
Copy link
Owner

evant commented Sep 10, 2024

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 evant added the enhancement New feature or request label Sep 10, 2024
@evant
Copy link
Owner

evant commented Sep 10, 2024

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
Copy link
Contributor Author

vRallev commented Sep 10, 2024

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
Copy link
Owner

evant commented Sep 10, 2024

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
Copy link
Contributor Author

vRallev commented Sep 10, 2024

Ah, thanks for explaining. That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants