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

Snapshot release 13 breaks @Singleton #107

Closed
Trikke opened this issue Jan 22, 2015 · 18 comments
Closed

Snapshot release 13 breaks @Singleton #107

Trikke opened this issue Jan 22, 2015 · 18 comments

Comments

@Trikke
Copy link

@Trikke Trikke commented Jan 22, 2015

Since the snapshot release of yesterday, i cannot compile my Components anymore. I have a setup similar to the 'android-activity-graphs' example.

  • Module1 uses @provides @singleton for a number of objects
  • Component1 uses @singleton @component(modules = Module1.class)
  • Component2 extends from Component1 and uses @singleton @component(dependencies = Component1.class, modules = { Module2.class, Module3.class })
  • Component3 is custom scoped with @PerActivity and uses @component(dependencies = Component1.class, modules = ActivityModule.class)

Since today I cannot compile anymore, with the following:
/src/main/java/di/Component2.java
Error:(17, 12) error: This @singleton component cannot depend on scoped components:
@singleton di.Component1
/src/main/java/di/Component3.java
Error:(17, 12) error: This @singleton component cannot depend on scoped components:
@singleton di.Component2

It seems @singleton suddenly doesn't likes itself anymore.

@Trikke
Copy link
Author

@Trikke Trikke commented Jan 22, 2015

I quickly made a simple project to test and simplified my setup. I get the error below as soon as I create the DataComponent, and have AppComponent use it as a dependency. If i only use AppComponent and ActivityComponent ( the latter depends on the former ), i have no issues.

So I'm either doing something wrong entirely and Snapshot 12 didn't mind ( most probable ), or there's a new issue in Snapshot 13.

Error:(36, 14) error:.di.ActivityComponent depends on scoped components in a non-hierarchical scope ordering:
@singleton di.DataComponent
@singleton di.AppComponent
@qast.di.PerActivity di.ActivityComponent

@chrisjenx
Copy link

@chrisjenx chrisjenx commented Jan 22, 2015

Similar issue, If you want more than 2 Components deep dagger is unusable.

@JakeWharton
Copy link

@JakeWharton JakeWharton commented Jan 22, 2015

This is by design.

@chrisjenx
Copy link

@chrisjenx chrisjenx commented Jan 22, 2015

@JakeWharton I appreciate that, looking through #96, its to make components stricter. It just makes things like this impossible:

@dagger.Component(dependencies = {ActivityComponent.class, AppComponent.class}, modules = ScreenModule.class)
@PerScreenScope
interface ScreenComponent {}
//--
@dagger.Component(dependencies = AppComponent.class, modules = ActivityModule.class)
@PerActivityScope
interface ActivityComponent {}
//--
@dagger.Component(modules = AppModule.class)
@Singleton
interface AppComponent {}

This was done to stop chances of depending on different lifecycled scopes (e.g. @PerActivity scope getting cleaned up before @PerScreen.) Which I understand.
But then I wish hierarchy component injection worked. (injecting something in the @PerScreen throws "May not reference binding with difference scopes")

Unless I am missing something? Docs are pretty light right now, but I guess thats due to the API changes.

@Trikke
Copy link
Author

@Trikke Trikke commented Jan 22, 2015

@JakeWharton, care to explain or point me to some resources on how to either set up multiple components? ( Imagine I'd like to have modules and components per seperate layer (data/business/...))

I've read @chrisjenx's explanation and understand the choice, but it seems to me that both components in the @singleton scope (or any same scope) should be able to depend on one-another?

@JakeWharton
Copy link

@JakeWharton JakeWharton commented Jan 22, 2015

Two components with the same scope can break scoping.

From your example:

Component1 c1 = Dagger_Component1.create();
Component2 c2_a = Dagger_Component2.builder().component1(c1).build();
Component2 c2_b = Dagger_Component2.builder().component1(c1).build();

c1 has singletons which are used across c2_a and c2_b but the singletons from Component2 get separate instances in c2_a and c2_b.

@chrisjenx
Copy link

@chrisjenx chrisjenx commented Jan 22, 2015

@Trikke I agree with what happened with Cyclic dependencies that will stop potential cases where you get multiple singletons. If you want to seperate your Data/Business logic, I use different Modules.

@JakeWharton if thats the case surely this should be acceptable:

Component1 c1 = Dagger_Component1.create();
Component2 c2 = Dagger_Component2.builder().component1(c1).build();
Component3 c3 = Dagger_Component3.builder().component2(c2).build();

c3.inject(ClassWithDependencencyInComponent1 classWithDep);

As you can build multiple c3's off one c2's which in turn will have a route to the c1 singletons?

@JakeWharton
Copy link

@JakeWharton JakeWharton commented Jan 22, 2015

@chrisjenx Yep. I wasn't disagreeing with your use case. Not at a place where I can test it right now.

@chrisjenx
Copy link

@chrisjenx chrisjenx commented Jan 22, 2015

@JakeWharton Ahh OK, My use case throws up in my face at the moment then. Hopefully a bug or me doing it wrong...

Error:(48, 5) error: ui.mortar.ActivateAddCard.Component scoped with @qualifiers.PerScreen may not reference bindings with different scopes:
@Singleton class io.database.DatabaseManager
//---

(That component depends on an Activity component, which depends on the App component)

@Trikke
Copy link
Author

@Trikke Trikke commented Jan 22, 2015

@chrisjenx , I already had different modules for separation of data/logic, and had components laid out in the same way. I'm still new to this stuff, so I'll have to read up on how to structure components properly. Although i think i had practically the same use case as you described above as something that should be possible.

@daverix
Copy link

@daverix daverix commented Jan 23, 2015

I ended up using different scopes for each of my subcomponents while my main component is annotated with @Singleton. Is this how you should do it or is there a way to not having to create scopes for each component?

@Singleton
@Component(modules = DatabaseModule.class)
public interface MainComponent {
    DatabaseProvider databaseProvider();
}

@OrderScope
@Component(dependencies = MainComponent.class, modules = OrderModule.class) 
public interface OrderListComponent {
    void inject(OrderListActivity activity);
}

@ScanningScope
@Component(dependencies = MainComponent.class, modules = ScanningModule.class)
public interface ScanningComponent {
    void inject(ScanActivity activity);
}
@cgruber
Copy link

@cgruber cgruber commented Jan 26, 2015

A few things -

Firstly, greg is working out a more flexible sub-component approach for this. It'll be cleaner, I think.

If you're going to do subcomponents in three levels, and you want to shuttle your singletons to the bottom layer, in the current code, just have your middle-tier component extend the application-level component. THis will expose those bindings to the lower-tier component without requring that you have your lower-tier depend on two scoped components.

@dagger.Component(dependencies = ActivityComponent.class, modules = ScreenModule.class)
@PerScreenScope
interface ScreenComponent {}
//--
@dagger.Component(dependencies = AppComponent.class, modules = ActivityModule.class)
@PerActivityScope
interface ActivityComponent extends AppComponent {} // <---- note here, the pass-through contract.
//--
@dagger.Component(modules = AppModule.class)
@Singleton
interface AppComponent {}

if you do this, then all the contract of AppComponent is visible to ScreenComponent via ActivityComponent. then you don't have to do the multiple dependencies (which are disallowed)

That said, I think the forthcoming @SubComponent approach will make this a little cleaner, and with fewer methods, etc. But for now, the above should be a reasonable way to go.

@cgruber
Copy link

@cgruber cgruber commented Jan 26, 2015

Also, during migration, you can disable the "singleton can't depend on singleton" bit with an annotation processor flag -Adagger.disableInterComponentScopeValidation=warning (or none). It is intended as a migration aid from dagger 1 so please don't rely on it, as it may not be there forever. It doesn't disable all validations, but should at least permit you to do the singleton->singleton stuff while you migrate to separate meaningful scoping annotations.

@chrisjenx
Copy link

@chrisjenx chrisjenx commented Jan 27, 2015

@cgruber Thanks for that, thats pretty much what I have done, I have Dependent interfaces for each activity scope which expose the singleton and app scopes up the tree.

@SubComponent sounds better, as there feels like way too much boilerplate right now.

I'll stay posted.

@cxzhang2
Copy link

@cxzhang2 cxzhang2 commented Aug 1, 2016

Any update on this? Is it sane to want the access controls of component dependencies and the transitive-ness of @Subcomponents?

i.e., some flag to toggle for Subcomponents to honor the provision methods of the parent component interface (and thus restrict access to non-exposed provisions in parent modules)?

@laiyifeng77
Copy link

@laiyifeng77 laiyifeng77 commented Aug 15, 2016

is there a final solution to handle this requirement?i mean the 3-level Component dependencies.

@ronshapiro
Copy link

@ronshapiro ronshapiro commented Aug 15, 2016

@cxzhang2 you can use the default java accessibility system to block provisions to be "passed" to a child subcomponent. I.e. if package com.example.foo has the @Component and uses a package-private annotation @Private, then a subcomponent in com.example.bar won't be able to access provisions from the parent since Java won't allow it to reference com.example.foo.Private. That's what we've been recommending.

@cxzhang2
Copy link

@cxzhang2 cxzhang2 commented Aug 15, 2016

@ronshapiro I see, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.