google / dagger

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

Component factories (an alternative to builders) #1317

Closed cgdecker closed 5 years ago

cgdecker commented 6 years ago

We're planning on adding a factory alternative to component builders to Dagger. A design document for the feature is available here.

This bug is mostly for tracking the feature and a request for feedback on the proposed API.

JayNewstrom commented 6 years ago

@cgdecker Could you make the proposal public?

cgdecker commented 6 years ago

Sorry, should be accessible now (changed the link).

ZacSweers commented 6 years ago

I've read through the doc and it looks fine to me. I do kind of wish there was some other less-ceremonious way to expose the factory since you can only have one and it's just there to designate @BindsInstance parameters. Something that let you do this instead

FooComponent fooComponent = DaggerFooComponent.create(bar);
Foo foo = fooComponent.foo();

rather than

FooComponent fooComponent = DaggerFooComponent.factory().newFooComponent(bar);
Foo foo = fooComponent.foo();

Maybe the Factory can be similar to @Binds and just there for processor signaling and discarded at runtime (and cleaned up by R8/proguard-like tools)

Then you'd have basically this

// Definition
@Component(modules = …)
interface FooComponent {
  Foo foo();

  // All of this is only here for compile-time use
  @Component.Factory
  interface Factory {
    FooComponent newFooComponent(@BindsInstance Bar bar);
  }
}

// Usage
FooComponent fooComponent = DaggerFooComponent.create(bar);
Foo foo = fooComponent.foo();

There might be some people that argue that makes testing harder because of the static method, but I'd probably argue that they're already testing the wrong thing if they're trying to mock generated code ¯\_(ツ)_\/¯.

The other benefit of having the intermediary factory could be allowing for custom naming, but that seems a little thing and irrelevant since you can only have one anyway.

ZacSweers commented 6 years ago

Hmm, I see a good use for having the factory interface in #1318 if you're injecting it I suppose and want to hide the Dagger parts of it. If that's the niche case though, I'd want that to be an optionally supported API rather than the one I use 95% of the time. Is there any sense of how often each use case would be used?

cgdecker commented 6 years ago

The case where you're calling DaggerFooComponent.factory() should be relatively rare, because you should only ever need to do that for root components, and in general subcomponents greatly outnumber root components (a simple check of use of the annotations in Google shows there to be between 6 and 7 times as many subcomponents as root components).

For subcomponents, the issue of needing to call a static method to get a factory doesn't even come up; you either need to just inject the factory or you need to call an instance method on the parent component. Injecting the factory is the preferred thing to do there.

Something that was considered but rejected at least for now was, for root components, taking the method defined by the factory type and creating a static method with the same signature, Javadoc and everything on the generated Dagger class. That seems to be essentially what you're asking for. We decided not to do that for a couple reasons:

  1. The root component case is actually much less common, as mentioned above.
  2. It doesn't make the call site much simpler; it just eliminates a .factory().
  3. It breaks the ability to use an IDE or other tool to find places where the factory's method is called, because users would be calling a completely unrelated (as far as cross-references are concerned) static method on another type.
ZacSweers commented 6 years ago

Makes sense to me!

cgdecker commented 6 years ago

Oh, by the way: another thing you can pretty easily do if you want call sites to look nicer is this:

@Component
abstract class MyComponent {
  static MyComponent create(...) {
    return DaggerMyComponent.factory().create(...);
  }

  @Component.Factory
  interface Factory {
    MyComponent create(...);
  }
}

Then you can just call MyComponent.create(...) wherever you want to instantiate it without call sites needing to even reference the generated type. Obviously the same thing can already be done today with builders (using the builder inside of your method) and requires hand writing a bit of additional code, but it's an option.

cgdecker commented 5 years ago

This feature is available at HEAD and will be in the next Dagger release. https://google.github.io/dagger/api/latest/dagger/Component.Factory.html

marxhendrik commented 5 years ago

In the Documentations https://google.github.io/dagger/api/latest/dagger/Component.Builder.html and https://google.github.io/dagger/api/latest/dagger/Component.Factory.html it is mentioned that the "Dependencies" have to be passed as arguments. But in the example the passed arguments are inside the "modules".

So should it really be

@Component(modules = {BackendModule.class, FrontendModule.class})

and

Builder backendModule(BackendModule bm); Builder frontendModule(FrontendModule fm);

Or rather

@Component(dependencies = {BackendModule.class, FrontendModule.class}, modules ={MyModule.class})

?

Maybe I misunderstand something, but we are definetely not passing the modules as dependencies to our Subcomponent.Builders and it works as intended.

cgdecker commented 5 years ago

Component dependencies are something different than modules... see https://google.github.io/dagger/api/latest/dagger/Component.html#component-dependencies.

marxhendrik commented 5 years ago

Yes, I seem to have mixed up modules and components in my line. My question is more this: Why do I need to provide bm and fm as parameters in the example in the docs:

@Component(modules = {BackendModule.class, FrontendModule.class})
 interface MyComponent {
   MyWidget myWidget();

    @Component.Builder
   interface Builder {
     Builder backendModule(BackendModule bm);
     Builder frontendModule(FrontendModule fm);
      @BindsInstance
     Builder foo(Foo foo);
     MyComponent build();
   }
 }

I made an example of what I mean. I do not provide any modules to the builder and the BackendModule provides Something to Bar:

@Component(modules = [BackendModule::class, FrontendModule::class])
interface MyComponent {

     fun inject(bar: Bar)

    @Component.Builder
    interface Builder {
        @BindsInstance
        fun bar(bar: Bar): Builder
        fun build(): MyComponent
    }
}

@Module
class BackendModule {
    @Provides
    fun provideSomething(bar: Bar) = Something(bar)
}

@Module
class FrontendModule

class Something(val bar: Bar)

class Bar {

    @Inject
    lateinit var something: Something

    init {
        DaggerMyComponent.builder().bar(this).build().inject(this)
        println("works: $something")
    }
}

So this would print the object when I create a new Bar() somewhere.

cgdecker commented 5 years ago

Ah, ok I see what you're asking. The answer is that for many modules, Dagger doesn't need you to pass an instance in when creating the component. In your example, Dagger can create an instance of BackendModule itself by calling its no-arg constructor, so there's no need for you to instantiate it. The main case where you'd actually need to pass a module in is when you have a module with a constructor that has parameters, so you need to instantiate it first for Dagger to use it. But it should be very rare that that would be needed; @BindsInstance is usually a better solution for cases where you might do that.

marxhendrik commented 5 years ago

Alright, thanks for clarifying!