google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.42k stars 2.01k forks source link

remove need for "empty" components #100

Closed andaag closed 8 years ago

andaag commented 9 years ago

Hi

We previously had one huge activity scoped component that handled pretty much everything, with dagger2 we wanted to make components per feature where that's possible. This allows us to make a lot of code package local instead of public. This resulted in .. 42 component classes, slightly more than we had hoped.

A lot of these just exist to inject very basic things, some base stuff that we use almost everywhere. For example:

@Component(dependencies = FinnApplicationComponent.class)
interface LastSearchesComponent {
    void inject(LastSearchesContentProvider target);
]

Would it be possible to annotate LastSearchesContentProvider with something to generate this file rather than creating seperate files for it? Our only alternative is to make 50+ classes public and create inject functions for them in FinnApplicationComponent. This clutters the code and produces a ton of package cycles.

marwinxxii commented 9 years ago

Hello, I've been thinking on a similar problem. Based on your example, is there really need to create a component? May be it can be done as following:

public class LastSearchesContentProvider {
  //some fields to be injected

  //mark that fields should be injected in constructor
  @Inject
  LastSearchesContentProvider() {}
}

public class MyActivity extends Activity {
  @Inject LastSearchesContentProvider mSearchesProvider;

  public onCreate(Bundle b) {
   ActivityComponent ac = Dagger_ActivityComponent.builder()
     .finnApplicationComponent(...)
     .build();
    ac.inject(this);
  }
}

@Component(dependencies=FinnApplicationComponent.class)
public interface ActivityComponent {
  void inject(MyActivity activity);
}

As I understand, Provider's dependencies and Provider itself (as dependency) would be injected as long as they can be found in component (ActivityComponent in example).

andaag commented 9 years ago

That doesn't work. If I change the LastSearchesContentProvider from:

@Singleton
@Component(dependencies = FinnApplicationComponent.class)
interface LastSearchesComponent {
    void inject(LastSearchesContentProvider target);
}

to inject(ParentClass target); the graph isn't correctly built, and the inject method does not inject the needed classes.

marwinxxii commented 9 years ago

What I meant is, get rid of method in component (and therefore component itself)

void inject(LastSearchesContentProvider target);

And use constructor injection

public class LastSearchesContentProvider {
  private final A mDependencyA;
  private final B mDependencyB;

  //mark that fields should be injected in constructor
  @Inject
  LastSearchesContentProvider(A a, B b) {
    this.mDependencyA = a;
    this.mDependencyB = b;
  }
}

And inject this provider where needed.

andaag commented 9 years ago

Hm, I can probably do that for an awful lot of classes.. The contentprovider not being one of them of course, since it's constructed by the system and needs an empty constructor ;)

andaag commented 9 years ago

Looking at my code the components are required in many places due to contentproviders/activities/fragments, all of which require empty constructors.

marwinxxii commented 9 years ago

You can have default empty constructor with @Inject annotation. But, this object can't be created directly (if you want to avoid manual inject(...) invocation), only injected. If it's a system component like activity, then yes, only way is to create component, I suppose.

andaag commented 9 years ago

I'd imagine that's the most used case for most of the android users though. Activities/Fragments/Views/Contentproviders/Actionproviders are all built around empty/preset constructors, and will be constructed by the system. Hence the need for quite many components ;)

marwinxxii commented 9 years ago

I agree that it's most used case. In my apps there no components which consist only of one activity/fragment with one inject method, so I haven't experienced the problem you've described. May be such components will occur later. 15.01.2015 10:17 пользователь "andaag" notifications@github.com написал:

I'd imagine that's the most used case for most of the android users though. Activities/Fragments/Views/Contentproviders/Actionproviders are all built around empty/preset constructors, and will be constructed by the system. Hence the need for quite many components ;)

— Reply to this email directly or view it on GitHub https://github.com/google/dagger/issues/100#issuecomment-70048494.

Alexander-- commented 9 years ago

Is there any progress on this? I imagine, this could be solved by supporting sort of profiles: predefined Dagger configs with lists of classes, that can't be created (and are illegal to use) in object graphs and should have components automatically created for them (Activities and ContentProviders on Android, Servlets in server environments etc.) The question is: how does one specify, which modules to use when creating each in typesafe way? Would it be possible to do that with something like old ObjectGraph, but with Oracle Tree Api instead of reflection for validation?

marwinxxii commented 9 years ago

If you have a number of components with one inject method and with same number of modules, than you can create base class (activity, fragment, etc.) and base component injecting instances of this base class.

Alexander-- commented 9 years ago

@marwinxxii, that'd be cool, but not really related to this issue. Similar Activities (with same dependencies) can be just refactored into single activity with multiple fragments. The real problem appears in any huge application, once number of framework-managed objects becomes big enough, that writing component for each one turns annoying. The annoying part is: invocation of inject method of old ObjectGraph already contained all information necessary to generate an empty component at compile time, it just has to be extracted somehow.

cgruber commented 9 years ago

Ok - I am not 100% following the need here - I think this would really benefit from a little example project - one that may not work, but shows the shape of the code you would wnat to see, so we can figure out what that looks like.

lukaspili commented 9 years ago

Had the same issue as @andaag when implementing Dagger2 in my android apps. Not a big deal, but ended up writing a small annotation processor that generates simple components: https://github.com/lukaspili/auto-dagger2

Is this something we could see in Dagger2, or is it out of scope?

cgruber commented 9 years ago

It's been discussed, and it's not 100% off the table, but we're seeing what evolves in the android and server spaces around this before deciding what to implement in dagger core.

On Sat, 30 May 2015 at 15:19 Lukas Piliszczuk notifications@github.com wrote:

Had the same issue as @andaag https://github.com/andaag when implementing Dagger2 in my android apps. Not a big deal, but ended up writing a small annotation processor that generates simple components: https://github.com/lukaspili/auto-dagger2

Is this something we could see in Dagger2, or is it out of scope?

— Reply to this email directly or view it on GitHub https://github.com/google/dagger/issues/100#issuecomment-107094707.

lukaspili commented 9 years ago

Ok, makes sense. Thanks for the details.

netdpb commented 8 years ago

I don't think we're going to support this specific request. As @lukaspili found, it's easy enough to write a processor that does this if you want to, but I'm not sure that's the pattern everyone should use.

It's not difficult for each of your activity or fragment classes to have a nested component interface, which can either be used as a supertype of one big activity component or annotated with @Component itself:

class LastSearchesContentProvider extends ContentProvider {
  @Component(dependencies = FinnApplicationComponent.class)
  interface Injector {
    void inject(MyActivity myActivity);
  }
}

We are planning to write documentation on the different ways to construct an Android application with Dagger, and the tradeoffs. We'll definitely discuss the issues you're raising here.

cgruber commented 8 years ago

Frankly, you could even (if you really felt the need to reduce two more lines of code) make

interface GenericInjector<T> {
  void inject(T t);
}

And then do

class LastSearchesContentProvider extends ContentProvider {
  @Component(dependencies = FinnApplicationComponent.class)
  interface Injector extends GenericInjector<MyActivity> {}
}

Though frankly, I'm not even sure that it's worth the two lines, since it reduces the readability at the declaration site and makes it require more implicit knowledge when reading that code.

c.

On Tue, 8 Dec 2015 at 14:25 David P. Baker notifications@github.com wrote:

I don't think we're going to support this specific request. As @lukaspili https://github.com/lukaspili found, it's easy enough to write a processor that does this if you want to, but I'm not sure that's the pattern everyone should use.

It's not difficult for each of your activity or fragment classes to have a nested component interface, which can either be used as a supertype of one big activity component or annotated with @Component itself:

class LastSearchesContentProvider extends ContentProvider { @Component(dependencies = FinnApplicationComponent.class) interface Injector { void inject(MyActivity myActivity); } }

We are planning to write documentation on the different ways to construct an Android application with Dagger, and the tradeoffs. We'll definitely discuss the issues you're raising here.

— Reply to this email directly or view it on GitHub https://github.com/google/dagger/issues/100#issuecomment-163037361.

netdpb commented 8 years ago

That works as well. It means you can't use those types as supertypes of a big component that implements them all, though, since one type can't implement both GenericInjector<Foo> and GenericInjector<Bar>.

cgruber commented 8 years ago

True. As I said, there's limits to its usefulness. I see that pattern, at best, useful for an android application scenario where you are making One individual subcomponent per activity.

On Wed, 9 Dec 2015 at 06:59 David P. Baker notifications@github.com wrote:

That works as well. It means you can't use those types as supertypes of a big component that implements them all, though, since one type can't implement both GenericInjector and GenericInjector.

— Reply to this email directly or view it on GitHub https://github.com/google/dagger/issues/100#issuecomment-163281293.