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

Why do we need to check if a component depends on more than one scoped component? #1414

Closed
BraisGabin opened this issue Feb 4, 2019 · 7 comments · Fixed by #1766
Closed
Assignees

Comments

@BraisGabin
Copy link

Why does Dagger need to check if a compomnent depends on more than one scoped component? You can bypass this restriction using things like this: #1225 (comment) and they don't seem a bad pattern. So, does Dagger still need this check?

@nlgtuankiet
Copy link

Same issue, same workaround, and the same question
I have done that in DataComponent
Not sure why we have this restriction and how the workaround affects the dependency but so far so good.

@grandstaish
Copy link

I've just run into this problem too, and was hoping to provide some context as to why I want this check removed (if it's safe to remove, that is!)

I'm thinking about how to improve build times in our Android application, which currently has a monolithic dagger graph all built in our top-most Gradle module. Instead I'd like to have kapt/dagger run in smaller parallelizable chunks: that is, I'd like to have a component per feature module.

Attempting this, you run into this limitation/error very quickly. E.g. imagine I have a feature named :settings. It has SettingsComponent. I've also got NetworkingComponent and DatabaseComponent. My SettingsComponent uses both the database and networking features, both of which should have scoped objects (e.g. my http client, my database instance, and so on). I want SettingsComponent to have component dependencies on NetworkingComponent and DatabaseComponent, but I get this error.

Most of the features in our app will depend on both networking and the database, as well as potential other scoped components! This seems like an unnecessary limitation. Is there any reason it is there, or is it due to be removed?

@ataraxus
Copy link

We just started to investigate dagger for breaking up our monolithic app. I'm just right now able to grasp what i'm run into.

Since we originally planned to structure our app like @grandstaish and till right now i was thinking we just did something wrong semantically. Is this issue going to be resolved? Or what exactly is the proposed architecture? To put everything top level in modules?

I do understand the issue regarding the diamond inheritance issue and the downsides, but i don't see how one should otherwise compose the dependency graph. again, sorry just starting with dagger, maybe there is a solution/error hiding in plain sight.

@roded
Copy link

roded commented Oct 12, 2019

We're in the same boat as @ataraxus and @grandstaish. Some guidance from the dagger team would be very useful and much appreciated. Can this limitation be expected to be relaxed in future versions of dagger?

I'd also like to mention that we're using dagger for a non-Android application and liking it very much. This issue however limits the scale to which we can take advantage of dagger.

@trevjonez
Copy link

the diamond inheritance issue

maybe not really an issue here? if a type is exposed on more than one of the 'dependencies' interface it results in a [Dagger/DuplicateBindings] error.

import dagger.Component
import javax.inject.Inject

class Foo @Inject constructor(val bar: Bar)

class Bar

@Component(
  dependencies = [
    ParentGraphOne::class,
    ParentGraphTwo::class
  ]
)
interface RootComp {
  val foo: Foo

  @Component.Factory
  interface Factory {
    fun create(
      other1: ParentGraphOne,
      other2: ParentGraphTwo
    ): RootComp
  }
}

interface ParentGraphOne {
  val bar: Bar
}

interface ParentGraphTwo {
  val bar: Bar
}

compiled with 2.26 results in:

/Volumes/Source/dagger1414/build/tmp/kapt3/stubs/main/RootComp.java:6: error: [Dagger/DuplicateBindings] Bar is bound multiple times:
public abstract interface RootComp {
                ^
      @org.jetbrains.annotations.NotNull Bar ParentGraphOne.getBar()
      @org.jetbrains.annotations.NotNull Bar ParentGraphTwo.getBar()
      Bar is injected at
          Foo(bar)
      Foo is provided at
          RootComp.getFoo()

If it did exist in both but was only exposed in the api of one, maybe that could lead to issues, but at that point it is an implementation detail? Probably not anything that needs to be that concerning to the processor if we consider it a fairly advanced use of the tool which should™ imply the user knows the likely gotcha's of their approach?

Debugging multiple instance issues is usually fairly straight forward by a myriad of ways, and something as simple as a heap dump and list of classes that are scoped should be sufficient to spot issues.

obviously more advance static analysis tooling could be developed if the issue came up too frequently.

@Chang-Eric Chang-Eric self-assigned this Mar 9, 2020
@Chang-Eric
Copy link
Member

I think this is something we should support. I'll work on changing this.

@jamesaherneplay
Copy link

That would be most excellent. Struggling with this issue when implementing multi module and dagger

netdpb pushed a commit that referenced this issue Mar 11, 2020
…ile enforcing this can help prevent cases where people mistakenly do this and leak objects beyond their intended lifetime, that is already something that could be done with a single scoped dependency and there are valid use cases for wanting to create a component that is an intersection of two lifetimes.

Fixes #1414

RELNOTES=Allow multiple scoped component dependencies

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=300138087
netdpb pushed a commit that referenced this issue Mar 11, 2020
…ile enforcing this can help prevent cases where people mistakenly do this and leak objects beyond their intended lifetime, that is already something that could be done with a single scoped dependency and there are valid use cases for wanting to create a component that is an intersection of two lifetimes.

Fixes #1414

RELNOTES=Allow multiple scoped component dependencies

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=300138087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants