Assisted Injection Proposal

[yayaa]

This proposal is to have some sort of AssistedInjection support for Motif.

Motif already has an API for factory implementation to create child Scope with given dynamic variables, but it is restricted only to Scope creation. By extending this functionality to any other type, we can already achieve assisted instance creation via Motif.

Problem

Currently, it is not supported to provide an instance with dynamic variables to child scope. In order to provide that, you can create a separate instance that would be responsible for generating the instance you need and provide that object as static instance.

Example; Assuming a callback needs to hold on to an instance so that can notify back with given instance, that means a new instance is required per given item.

class SampleImplementation(
    private val dynamicData: DynamicData,
    private val logger: Logger
) : SampleInterface { }

@motif.Scope
interface SampleScope {
    fun createChildScope(callback: SampleInterface): ChildScope
}

class ConsumerExample(
    private val sampleScope: SampleScope,
    private val logger: Logger
) {

    fun usage(dynamicData: DynamicData) {
        val childScope = sampleScope.createChildScope(
            new SampleImplementation(dynamicData, logger)
        )
    }

}

This makes ConsumerExample to know about how to create an instance of SampleInterface, which violates inversion of control and also creates a direct dependency to Logger even though it is not needed besides creating the instance for the callback.

Workaround

In order to work around this, we can create a factory implementation that can create an instance of the callback lazily and provide this factory in DI so that ConsumerExample can have the factory and generate the instance it needs.

interface SampleInterfaceFactory {
    fun create(dynamicData: DynamicData): SampleInterface
}

@motif.Scope
interface SampleScope {

    @motif.Objects
    open class Object {

        fun sampleInterfaceFactory(logger: Logger): SampleInterfaceFactory {
            return dynamicData -> SampleImplementation(dynamicData, logger)
        }

    }

}

class ConsumerExample(
    private val sampleScope: SampleScope,
    private val sampleInterfaceFactory: SampleInterfaceFactory
) {

    fun usage(dynamicData: DynamicData) {
        val childScope = sampleScope.createChildScope(
            sampleInterfaceFactory.create(dynamicData)
        )
    }

}

Even though this is doable, it still introduces a lot of boilerplate code.

Proposed Solution

Considering we already have enough information in DI tree, we can benefit from Assisted Injection and pass only the dynamic dependencies and have the rest provided by Motif.

@motif.Scope
interface SampleScope {

    fun createChildScope(callback: SampleInterface): ChildScope

    fun createSampleImplementation(dynamicData: DynamicData): SampleImplementation

}

class ConsumerExample(private val sampleScope: SampleScope) {

    fun usage(dynamicData: DynamicData) {
        val childScope = sampleScope.createChildScope(
            sampleScope.createSampleImplementation(dynamicData)
        )
    }

}

Motif should be able to create an instance of SampleImplementation with give DynamicData given that it already has a Logger instance in the DI tree. So the implementation would need to diff given dynamic dependencies with constructor dependencies and satisfy remaining dependencies from DI.

[andreasnomikos]

I do support this proposal.

Generating hybrid instances with a combination of dynamic and static dependencies is a common enough use case that should be considered for support from any DI framework. Creating an extra factory class or lambda/SAM just to wrap the static dependencies is wasteful both from redundant code generation perspective as well as from class count/method count.

Extending Motif child method api seems to fit this use case pretty well though the name "child" will become a bit confusing now since we will be generating non-motif children from a similar declaration. You could view the SampleImplementation as . an optimized "child-scope" of the SampleScope that provides a single type implementation but its a stretch.

We could consider the following declaration style if we want to increase clarity, but that might be confusing for users in case they want to define these declarations within interfaces in other libraries.

@motif.Scope
interface SampleScope extends SampleScope.Children, SampleScope.Providers {

    @motif.Children
    interface Children {
      fun childScope(callback: SampleInterface): ChildScope
    }

    @motif.Providers
    interface Providers {
      fun sampleImplementation(dynamicData: DynamicData): SampleImplementation
    }
}

[Leland-Takamine]

I think an API like the initial proposal makes a lot of sense. The annotation-based API above I feel would be a to big of a departure from the existing API and introduces too much noise, so let's focus on the initial proposal for now.

It's clear that this API cuts down on boilerplate for a certain use-case. The piece to consider now is whether it's worth adding a new API. That all depends on how much value we believe this is adding, so I'll ping folks internally at Uber to make sure to get more eyes on this proposal. I'd like to get some more perspectives.

Separately, the Java API is only half of the story here. We need to also be thinking about what the documentation will look like. @yayaa It would be great if you could add a "Reference" section (eg: Access Methods) and a "User Guide" section (eg: Convenience Methods) to this proposal with a rough idea of what the documentation would look like. This will give us a better idea of how intuitive the new API actually is and how well it fits in with the rest of the library.

[mcassiano]

I love the idea of having assisted injection in Motif, but the proposed solution would mean allowing patterns we forbid today (useful to avoid mistakes like forgetting to add motif.Scope to child methods). My point is, implementing this proposal as is will introduce ambiguity to the current API.

[andreasnomikos]

How about something in line with the Creatable extention we added recently

Declaration

@motif.Scope
interface SampleScope extends Assisted<SampleScope.Assisted> {

    fun childScope(callback: SampleInterface): ChildScope

    interface Assisted {
      fun sampleImplementation(dynamicData: DynamicData): SampleImplementation
    }
}

interface Assisted<D> {
  val assisted: D 
}

Usage

scope.assisted.sampleImplementation(dynamicData);

[Leland-Takamine]

scope.assisted.sampleImplementation(dynamicData);

This doesn't seem consistent with the rest of the API. I'd like to try to avoid nesting like this as it adds noise to both the definition and the call site.

[andreasnomikos]

Fair point but then you have to deal with the ambiguity of child method vs assisted method. scope.child() vs scope.assisted() Unless you want to go for convention over configuration and demand that all method names start with "assisted" e.g scope.assistedFoo()

[TonyTangAndroid]

I usually deal such case through binding SampleInterface with SampleImplementation in child scope and let Motif itself to figure out what's need to be provided to create the SampleImplementation. Here is a pull request to demonstrate all the implementation.

The consumer should not be aware of SampleImplementation nor the SampleInterface. The consumer's responsibility is to create the child scope and enjoy the access method exposed by the child scope.

@motif.Scope
public interface ChildScope {

    SampleInterface sampleInterface();

    @motif.Objects
    abstract class Objects {

        abstract SampleInterface bind(SampleImplementation impl);

        abstract SampleImplementation impl();
    }
}

@Test
public void build_childScope() {
        DynamicData dynamicData1 = new DynamicData();
        DynamicData dynamicData2 = new DynamicData();
        {
            ChildScope childScope = new SampleScopeImpl().childScope(dynamicData1);
            assertThat(childScope.sampleInterface().dynamicData()).isEqualTo(dynamicData1);
        }

        {
            ChildScope childScope = new SampleScopeImpl().childScope(dynamicData2);
            assertThat(childScope.sampleInterface().dynamicData()).isEqualTo(dynamicData2);
        }
}

[yhartanto]

val childScope = sampleScope.createChildScope(
            sampleScope.createSampleImplementation(dynamicData)
        )

The problem with the above code is that you are still exposing createSampleImplementation to the createChildScope caller. I think it should be more like:

@motif.Scope
interface SampleScope {

    fun createChildScope(dynamicData: DynamicData): ChildScope

    @factory //or other name like dynamic
    fun createSampleImplementation(dynamicData: DynamicData): SampleImplementation

}

Then caller can just call

val childScope = sampleScope.createChildScope(dynamicData)

[yayaa]

@Leland-Takamine so I would think of something like this

AssistedFactory Methods

An AssistedFactory method will provide the dependencies that are already in the graph and only require the dynamic dependencies to create an instance.

Assuming a Bar class with 2 required dependencies, one as a static dependency that is provided in the scope objects already and second as a dynamic dependency that is required to create a new instance.

@motif.Scope
interface FooScope {
  Bar bar(DynamicDependency a);
}

Spec

• Declared on top-level Scope interface
• Parameters define dynamic dependencies
• Returns any type

Dagger Notes Similar behavior can be accomplished with AssistedInjection

Convenience APIs

Some objects might require static and dynamic dependencies to create an instance. With AssistedFactory methods, you only need to provide dynamic ones, while Motif will provide the static ones.

class Bar {
   Bar(DynamicDependency a, StaticDependency b) {
        ...
   }
}

Considering the class definition above and having StaticDependency provided in scope objects, you can simply have an AssistedFactory method with only dynamicDependency.

@motif.Scope
interface FooScope {
  Bar bar(DynamicDependency a);

  @motif.Objects
  abstract class Objects {
    abstract StaticDependency staticDependency();
  }

}

[yayaa]

@yhartanto even though I like that API, there is a slight difference.

val childScope = sampleScope.createChildScope(dynamicData)

This would make dynamicData as part of childScope, so that would be exposed in the scope and any other dependency can have access to it, which is not the intention. Instead, only SampleImplementation class should know about this dynamicData.

---

I think @TonyTangAndroid your example has the same concern as I mentioned.

[andreasnomikos]

Given that Assisted is an extension to the core framework for convenience I am ok with adding a bit more textual "overhead" to the api at the definition and the declaration in order to resolve the ambiguity. This also provides us with a clear template on how to introduce other optional extensions to the framework while keeping the core of the framework api clean. e.g. You want to use extension capabilities on the scope you will have to go through an accessor method to access those capabilities in a delegate object.

Definition: Even considering the flat structure of the original proposal a good SW practice would be to logically group the child methods and the assisted methods together and add some clarification comments

class Scope {
// children
ChildScope childScope(DynamicDeps deps);
// assisted
AssistedInstance assistedInstance(AssistedDynamicDeps deps);
}

wrapping the assisted methods with an interface is not a stretch and also allows to use reusable interfaces of related types to export the factory methods to multiple scopes.

Usage: With the flat structure given a usage of

var child = scope.child(bar)
var instance = scope.instance(bar)

Its unclear what the instance method is returning. Scopes vs Instances as far as the framework is concerned are 2 distinct concepts so it would be helpful if the ambiguity was resolved. A good software practice would be to use a good naming convention like

var child = scope.childScope(deps)
var instance = scope.assistedInstance(deps)

at which point the proposed API is not that far off

var child = scope.child(deps)
var instance = scope.assisted.instance(deps)

Given that motif's design principle is an opinionated framework I am ok with pigeonholing the users into a specific usage in order to resolve ambiguities.

[Leland-Takamine]

@andreasnomikos Agree that the ambiguity is an issue. Today we have a bit of this already with access methods vs child methods (though there's already a precedent for that with Dagger's provision methods vs subcomponent factory methods). It's true though that the status quo doesn't justify adding additional ambiguity.

Would you or @yayaa be able to pull some rough numbers to help us make a decision here. Adding a new API adds non-trivial overhead to overall API complexity and ultimately education/onboarding so we'll need some numbers to back up our intuition on this.

[yayaa]

I also agree about the API ambiguity, but what do you think of using @Assisted annotation instead of nesting? @andreasnomikos

@motif.Scope
interface SampleScope {

    fun accessThings(): Things

    fun createChildScope(callback: SampleInterface): ChildScope

    @motif.Assisted
    fun createSampleImplementation(dynamicData: DynamicData): SampleImplementation

}

---

I am not sure if I understand what numbers you need @Leland-Takamine, could you please elaborate?

[Leland-Takamine]

Sorry I meant rough numbers on how often this pattern is used in the Uber monorepo for instance.

[andreasnomikos]

the challenge with @motif.Assisted is you need to also handle the following

interface Extension {
   fun createSampleImplementation(dynamicData: DynamicData): SampleImplementation
}

@motif.Scope
interface SampleScope extends Extension {

}

Since the Extension might be defined in a different library and reused between motif and non motif code we can't rely on anything being annotated on the interface

Either we go by convention no params > access method with params > assisted method return type is scope > child method

or by inner classes

[andreasnomikos]

@yayaa I have encountered another use case for this for root scopes that also implement builder interfaces with dynamic dependencies so I think that despite the ambiguity the initial api with access, child and assisted methods side by side is fine. @Leland-Takamine what would it take to move this forward ?

The only problem I see with the api is that the following is not possible

@motif.Scope
interface SampleScope {

    // bind to SampleImplementation
    fun createSampleImplementation(dynamicData: DynamicData): SampleInterface

}

[Leland-Takamine]

@yayaa Thanks for creating the sample documentation. It highlights an important point: Assisted injection is an advanced concept that is actually quite difficult to explain on paper. This means that the addition of this API not only increases the API surface area, but the added functionality itself is more complex than most of Motif's APIs.

One of Motif's fundamental design principles is simplicity. Specifically, Motif's primitives should be simple and easy to reason about. To answer your question @andreasnomikos the default decision is to limit API complexity (ie: no new APIs), and a reasonable workaround exists using Motif's existing primitives so it's a bit of a tough sell. In this case, the concept is compelling but we'd need to see how common this scenario is - if one of you can gather some data in the monorepo, we can continue the discussion.

[yayaa]

@Leland-Takamine right, I actually struggled while writing that down as well :) It is easy as a concept to explain "provide only what is not static and everything else will be provided by Motif if exist in DI tree" but it was difficult to put in the format on given API documentation :)

I understand the need for the number to justify the effort, but just to manage the expectations here - I don't really have that much time right now to do such analysis, but I'd like to keep the issue open so that I can come back to it once I have the time to invest.

Meanwhile if @andreasnomikos or anyone else has time to look into, please go ahead.