hmlongco / Factory

A new approach to Container-Based Dependency Injection for Swift and SwiftUI.
MIT License
1.87k stars 115 forks source link

Factory 2.0! Input on Factory's future is needed! #51

Closed hmlongco closed 1 year ago

hmlongco commented 1 year ago

Factory is evolving and I'm currently working on Factory 2.0 which adds true container-based dependency injection! This allows Factory to provide a variety of traditional dependency injection and service locator strategies.

class ContentViewModel: ObservableObject {

    // Old-style Factory 1.0 static service Locator
    let oldSchool = Container.oldSchool()

    // New Factory 2.0 shared service Locator
    let service = Container.shared.constructedService()

    // Constructor initialized from container
    let service2: MyConstructedService

    // Lazy initialized dependencies from passed container
    private let container: Container
    private lazy var service3: MyConstructedService = container.constructedService()
    private lazy var service4: MyServiceType = container.cachedService()

    // Annotated, keyPath-based dependency injection from default shared container
    @Injected(\.constructedService) var constructed

    // Annotated, keyPath-based dependency injection from a custom container
    @Injected(\MyContainer.anotherService) var anotherService

    // Constructor with passed container
    init(container: Container) {
        // construct from container
        service2 = container.constructedService()
        // save container reference for lazy resolution
        self.container = container
    }

}

Supporting containers has been one of my goals for awhile now, as it dramatically increases the environments and applications in which Factory can be used and considered.

But, as with most things, there's a catch: In order to do this the dependency definition has to change, making this a breaking change between 1.0 and 2.0.

Here's the existing mechanism.

extension Container {
    static let myServiceType = Factory<MyServiceType> { 
        MyService() 
    }
}

And here's a version of the same thing in Factory 2.0.

extension Container {
    var service: Factory<MyServiceType> {
        Factory(self) { MyService() }
    }
}

The Factory is now defined as a Container Member, and not a Container Class Member as is done today.

Note that three things must occur to make container Factory definitions work as expected.

  1. The Factory definition is now returned as a computed value.
  2. The Factory return type must be explicitly specified (e.g. Factory<MyServiceType>).
  3. The Factory requires a reference to its container. (self)

This adds a bit of extra code, but makes Factory much, much more powerful.

Note the basic definition could be written in several ways:

extension Container {
    var service1: Factory<MyServiceType> {
        Factory(self) { MyService() }
    }
    var service2: Factory<MyServiceType> {
        .init(self) { MyService() }
    }
    var service3: Factory<MyServiceType> {
        factory { MyService() }
    }
    var service4A: Factory<MyServiceType> {
        factory(scope: .shared) { MyService() }
    }
    var service4B: Factory<MyServiceType> {
        shared { MyService() }
    }
}

The first is fully explicit. The second uses the .init shorthand. The third uses a helper function on the container that returns the correctly bound Factory; and the fourth version demonstrates the same with the scope syntax added. The fifth variant adds a few more helpers that do the same thing as factory but specifically indicates the scope (unique, singleton, shared, etc.) at the same time.

At this point in time I'm considering going with the later option exclusively, as it ensures the correct binding and also requires the developer to explicitly consider and define the desired scope for a given factory.

extension Container {
    service6: Factory<MyServiceType> {
        unique { MyService() }
    }
    var service7: Factory<MyServiceType> {
        shared { MyService() }
    }
}

ParameterFactory would be going away as well, as it's easy to simply do...

extension Container {
    func parameterized(_ n: Int) -> Factory<ParameterService> {
        unique { ParameterService(count: n) }
    }
}

But updating and reseting services would work much as it does today.

    Container.shared.service.register { MockServiceN(8) }

Or it can be done as follows if you have access to the specific container. This version changes the behavior on that instance of that container and nowhere else.

    container.service.register { MockServiceN(8) }

This leaves me a few questions.

It's possible to make Factory 1.0 and 2.0 work together, but I'm inclined to deprecate the existing methodology as the two approaches differ somewhat in behavior, in how dependencies are managed, and, of course, supporting both the old and new code styles increases the library size.

So...

  1. Should Factory 1.0 be deprecated?
  2. Or should we accept that Factory 2.0 will be a breaking change and be done with it?
  3. And in any case, which definition style (1-4) shown above would be preferred? (Or just provide all of 'em?)

One drawback to the service4 style is that containers start to get "busy" with predefined helper functions (unique, shared, singleton, cached) in addition to the factory definitions.

I really want Factory to be the best DI system for Swift, and to do that I really need some help here. So leave a comment if you approve, disapprove, or have questions.

rydamckinney commented 1 year ago

So...

  1. Should Factory 1.0 be deprecated?

Sure, I don't see why not.

  1. Or should we accept that Factory 2.0 will be a breaking change and be done with it?

Yes, I think breaking changes such as the initializer changes in the discussion above are more than reasonable for a 2.0 release.

  1. And in any case, which definition style (1-4) shown above would be preferred? (Or just provide all of 'em?)

My preference would be to provide all of them.

In addition to the points noted above for 2.0 -- my main feature request: Dynamic, Container Scopes, resolved at runtime.

hmlongco commented 1 year ago

@rydamckinney Resolver has a .container scope that's basically a cache for the current Resolver instance.

That scope didn't make a lot of sense for Factory since it didn't really have containers, but I was in fact planning on adding that capability back to Factory 2.0.

Only in this case it applies to any scope. Basically that means if a container goes out of scope then any cached instances associated with it are also released. I mean, if the container goes away you're not going to be able to ask it for a new singleton, are you?

From a user's perspective, that means that each instance of container "X" appears to have its own complete set of scopes.

Or in other words, I think you're going to get your wish. ;)

ahmadmssm commented 1 year ago
  1. I like this way of definition:

    var service4A: Factory<MyServiceType> {
        factory { MyService() }
    }

    It makes sense but I have a suggestion, instead of passing the container scope, let's use a typed container for example, factory for an instance, shared for a singleton, and graph for graph, etc...

    var service4A1: Factory<MyServiceType> {
        factory { MyService() }
    }
    
     var service4A2: Factory<MyServiceType> {
        shared { MyService() }
    }
    
    var service4A3: Factory<MyServiceType> {
        graph { MyService() }
    }
    
      var service4A3: Factory<MyServiceType> {
        scoped(.myScope) { MyService() }
    }
  2. Regarding this point: ParameterFactory would be going away as well, I guess it is also a good idea, but why don't we change everything from variable to function just to keep all the styles of declaring a factory similar ?

    func service4A1() -> Factory<MyServiceType> {
        factory { MyServiceType() }
    }
  3. The only thing I don't like is accessing the container as a singleton instance using shared, I think it is better to expose container functions as static functions, as is, and hide the singleton accessor,.

hmlongco commented 1 year ago

It makes sense but I have a suggestion, instead of passing the container scope, let's use a typed container for example, factory for an instance, shared for a singleton, and graph for graph, etc...

I mentioned that above, "The fifth variant adds a few more helpers that do the same thing as factory but specifically indicates the scope (unique, singleton, shared, etc.) at the same time."

  1. Regarding this point: ParameterFactory would be going away as well, I guess it is also a good idea, but why don't we change everything from variable to function just to keep all the styles of declaring a factory similar ?

Property wrapper key paths depend on variables.

  1. The only thing I don't like is accessing the container as a singleton instance using shared, I think it is better to expose container functions as static functions, as is, and hide the singleton accessor,.

Factory 2.0 is multi-modal. You can use "pure" containers for DI, or you can use .shared and basically do a variant of what's done today. The shared variable ensures that you know what instance of what container is responsible for caching and registrations.

It's also required if you decide you want to use the annotation property wrappers keypaths as they need to be able to find an instance of a container to use.

ahmadmssm commented 1 year ago

I mentioned that above, "The fifth variant adds a few more helpers that do the same thing as factory but specifically indicates the scope (unique, singleton, shared, etc.) at the same time."

Just seconding your point πŸ˜„

Property wrapper key paths depend on variables.

You are right, I usually rely on constructor injection so I missed the injection using the property wrapper feature.

hmlongco commented 1 year ago

So in defining a 2.0-version Factory I could do...

    var sample1: Factory<MyServiceType> { Factory(self, scope: .shared) { MyService() } }

But that's clunky. I'd prefer to do...

    var sample2: Factory<MyServiceType> { Shared { MyService() } }

Which is a lot cleaner. The problem, however, is that the capitalized Shared object in that sample is in fact an inlinable function that basically returns Factory(self, scope: .shared, factory: factory) for you.

I could write it as follows...

    var sample3: Factory<MyServiceType> { shared { MyService() } }

But some stubborn part of me prefers the capitalized version. Anyone for or against?

bensLine commented 1 year ago

It's great to read up on the new plans, looking forward to it! And cool that you gather feedback here :)

I prefer .shared as static function to allow auto completion

var sample3: Factory<MyServiceType> { .shared { MyService() } }

That seems to me the swifty way. An uppercased Shared gives me the notion that it is a constructor rather than a function.

hmlongco commented 1 year ago

@bensLine The shared function works because it's an extension on the container and as such knows about the container.

    var sample9: Factory<MyServiceType> {
        shared { MyService() }
    }

A dot-shared would be a static on the Factory itself and as such it won't work. Or, rather, it would, but it would then need to be .shared(self) in order to pass the container reference.

    var sample9: Factory<MyServiceType> {
        .shared(self) { MyService() }
    }
hmlongco commented 1 year ago

Closing in on final syntax for Factory registrations, adding "builder" helpers for scopes and decorators.

// see below

Think this provides a good compromise between clarity and expressiveness. Thoughts?

hmlongco commented 1 year ago

A working version of 2.0 I in the develop branch...

ahmadmssm commented 1 year ago

I'm not a fan of defining a factory with a scope this way:

var sharedService: Factory<ServiceType> {
       factory { MyService() }.shared
}

I guess passing it as a type would be more elegant.

var sharedService: Factory<ServiceType> {
       factory(.shared) { MyService() }
}
ahmadmssm commented 1 year ago

So in defining a 2.0-version Factory I could do...

    var sample1: Factory<MyServiceType> { Factory(self, scope: .shared) { MyService() } }

But that's clunky. I'd prefer to do...

    var sample2: Factory<MyServiceType> { Shared { MyService() } }

Which is a lot cleaner. The problem, however, is that the capitalized Shared object in that sample is in fact an inlinable function that basically returns Factory(self, scope: .shared, factory: factory) for you.

I could write it as follows...

    var sample3: Factory<MyServiceType> { shared { MyService() } }

But some stubborn part of me prefers the capitalized version. Anyone for or against?

I prefer the non-capitalized version, makes more sense to me.

ahmadmssm commented 1 year ago

// Example of parameterized functional registration in a Factory 2.0 container

extension Container {
    var parameterService: ParameterFactory<Int, ParameterService> {
        ParameterFactory(self) { ParameterService(value: $0) }
    }
}

So you reverted from this ?

extension Container {
    func parameterized(_ n: Int) -> Factory<ParameterService> {
        unique { ParameterService(count: n) }
    }
}

I would prefer the second to be honest, it is cleaner than the first.

ast3150 commented 1 year ago

Would something speak against providing functions in the Container extension to modify the scope? Also, to me it feels weird to use a noun for a function. Could it be named as an action, as is common practice? This way you get natural readability, as in "this is a Factory making MyService"

Using your example:

// Example of basic registration in a Factory 2.0 container.
extension Container {
    var service: Factory<MyServiceType> {
        making { MyService() }
    }
}

// Examples of scoped services in a Factory 2.0 container
extension Container {
    var cachedService: Factory<MyServiceType> {
        makingCached { MyService() }
    }
    var singletonService: Factory<SimpleService> {
        makingSingleton { SimpleService() }
    }
    var sharedService: Factory<MyServiceType> {
        makingShared { MyService() }
            .decorated { print("DECORATING \($0.id)") }
    }
}

// Example of service with constructor injection that requires another service
extension Container {
    var constructedService: Factory<MyConstructedService> {
        making { MyConstructedService(service: self.cachedService()) }
    }
}

// Example of parameterized functional registration in a Factory 2.0 container
extension Container {
    var parameterService: ParameterFactory<Int, ParameterService> {
        ParameterFactory(self) { ParameterService(value: $0) }
    }
}
hmlongco commented 1 year ago

So you reverted from this ?

extension Container {
    func parameterized(_ n: Int) -> Factory<ParameterService> {
        unique { ParameterService(count: n) }
    }
}

I would prefer the second to be honest, it is cleaner than the first.

So did I, except it doesn't work. You need access to the factory to resolve it and pass in the parameter (service(5)). And written as a function you can't get to the Factory to register a new closure w/o calling the function with a parameter.

Turns out I originally wrote it that way for a reason.

hmlongco commented 1 year ago

Would something speak against providing functions in the Container extension to modify the scope? Also, to me it feels weird to use a noun for a function. Could it be named as an action, as is common practice? This way you get natural readability, as in "this is a Factory making MyService"

One could, but I was really trying to avoid polluting the container namespace with a lot of factory-specific helper functions for each and every scope. The modifier syntax helped eliminate that, came in handy for adding decorator, and will be useful for a few other things I have in mind.

Will consider the verb thing, but haven't found one I really like...

hmlongco commented 1 year ago

Consider...

extension Container {

    // Formally constructing Factory
    var service1: Factory<SimpleService> {
        Factory(self) { SimpleService() }
    }

    // Using .init shortcut
    var service2: Factory<SimpleService> {
        .init(self) { SimpleService() }
    }

    // Just container to do it...
    var service3: Factory<MyServiceType> {
        self { MyService() }
    }

}

Playing with options and thought the latercallAsFunction version was fun... if a bit mind bending.

Although it's actually starting to grow on me...

ahmadmssm commented 1 year ago

What about my other comments ? πŸ˜„

I'm not a fan of defining a factory with a scope this way:

var sharedService: Factory<ServiceType> {
       factory { MyService() }.shared
}

I guess passing it as a type would be more elegant.

var sharedService: Factory<ServiceType> {
       factory(.shared) { MyService() }
}

What about this point ? πŸ˜„

hmlongco commented 1 year ago

@ahmadmssm As mentioned, the modifier syntax helped eliminate a lot of the container namespace pollution, came in handy for adding decorator, and will be useful for a few other things I have in mind. I really don't want to start providing 15 different ways of doing the same thing.

ahmadmssm commented 1 year ago

So no hope to change it ?, because even from a convention perspective, it seems misleading a little bit and for me, the function param makes much more sense, but this is just my opinion.

hmlongco commented 1 year ago

Once again closing in on final syntax for Factory registrations, using SwiftUI-style modifiers for scopes and decorators.

// Example of formal registration in a Factory 2.0 container.
extension Container {
    var service: Factory<MyServiceType> {
        Factory(self) { MyService() }
    }
    var service2: Factory< MyServiceType > {
        .init(self) { MyService() }
    }
}

// Same as above, but asks current container to do the binding and make our Factory for us.
extension Container {
    var service: Factory<MyServiceType> {
        makes { MyService() }
    }
}

// Examples of scoped services in a Factory 2.0 container using scope modifiers
extension Container {
    var cachedService: Factory<MyServiceType> {
        makes { MyService() }.cached
    }
    var singletonService: Factory<SimpleService> {
        makes { SimpleService() }.singleton
    }
    var sharedService: Factory<MyServiceType> {
        makes { MyService() }
            .decorator { print("DECORATING \($0.id)") }
            .shared
    }
}

// Example of service with constructor injection that requires another service
extension Container {
    var constructedService: Factory<MyConstructedService> {
        makes { MyConstructedService(service: self.cachedService()) }
    }
}

// Example of parameterized functional registration in a Factory 2.0 container
extension Container {
    var parameterService: ParameterFactory<Int, ParameterService> {
        makes { ParameterService(value: $0) }
    }
}

Note that any factory modifiers added (.shared, .decorator, .etc) work on both Factory and ParameterFactory. This simplifies implementation internally and helps prevent the multiplication of helper functions on the Container.

Settled on makes for helper. Evaluating self was cute, but probably too confusing and didn't autocomplete correctly. This reads well, and even has a double meaning in that we're asking the Container to make a Factory that will make a dependency from the following closure.

ahmadmssm commented 1 year ago

One more suggestion, could we change make to create ? The library is called factory and create keyword is known when using factory pattern.

ernesto commented 1 year ago

With Factory 2.0 can be possible a dynamic resolver like koin library does? Ex: let service: XPTOProtocol? = container.get()

hmlongco commented 1 year ago

With Factory 2.0 can be possible a dynamic resolver like koin library does? Ex: let service: XPTOProtocol? = container.get()

Well, you could do something like...

let service: XPTOProtocol? = container.get(\.xptqService)

But why?

let service: XPTOProtocol? = container.xptqService()

Bottom line is that that sort of type-inference defeats the compile-time-safe rationale behind Factory. If you want blind registrations and resolutions look at Resolver.

kamaldeep-earnin commented 1 year ago

With 2.0, it seems like we'll have to write an initializer to pass on the container first. One of the benefits we were reaping of the 1.0 design was conciseness. We didn't had to write down an init function at all for a lot of our constructs. Also, if containers are gonna be on the heap, there could be chances of dynamic resolution failures. What are some of the benefits of allocating containers on the heap ?

Also, as far as the following syntax goes, it looks ambiguous compared to 1.0.

var cachedService: Factory<MyServiceType> {
    factory { MyService() }.cached
}

So, the way someone will read the above is - create a member variable that is a Factory returning type MyServiceType. To initialize an instance, use some magical factory helper (then there's Factory before that) within it and create the instance within the factory's closure. Then apply the scope decorator. It seems like we are trying too hard to skew the API design to follow SwiftUI coding style.

Then, the following is a different way of creating a Factory where self is passed to ParameterFactory compared to the previous approach where helper is used. This looks inconsistent as far as API design goes.

extension Container {
    var parameterService: ParameterFactory<Int, ParameterService> {
        ParameterFactory(self) { ParameterService(value: $0) }
    }
}

Would it make sense to create scope based Factory sub-classes rather than using factory helper ?

hmlongco commented 1 year ago

@kamaldeep-earnin You have to have a reference to a container in order to resolve the factory.If, for some reason, you were to extract a Factory and save it somewhere, it maintains an internal reference to its container. Either way, it's not going to fail.

If you want to use the current "service locator" design like let s = Container.myService() you can.

extension Container {
    static var myService: Factory<MyServiceType> {
        makes { MyService() }
    }

That said, unless dependencies are in a container you won't be able to grab them using the property wrappers. So the following is preferred...

extension Container {
    var myService: Factory<MyServiceType> {
        makes { MyService() }
    }

And then do a single search and replace and do let s = Container.shared.myService().

Which basically does the same thing as the static version while also allowing the shared container to be swapped and/or replaced.

The 2.0 changes exist to enable true container-based DI as well as the existing SL methodology.

As shown, the current "magical" helper name was....

extension Container {
    var cachedService: Factory<MyServiceType> {
        makes { MyService() }.cached
    }
    var parameterService: ParameterFactory<Int, ParameterService> {
        makes { ParameterService(value: $0) }
    }

And those were created to avoid the more formal Factory(self) {} or .init(self) {} initializers with the parameter needed to bind the factory to the container.

A lot of people seem to want...

extension Container {
    var cachedService: Factory<MyServiceType> {
        cached { MyService() }
    }
    var parameterService: ParameterFactory<Int, ParameterService> {
        shared { ParameterService(value: $0) }
    }

But I was trying to avoid autocompletion pollution with adding cached, shared, singleton, unique, custom() and so on such that half the things you see when you type "." on a container are factory helpers and not your own own factories. Adding scope-based helpers adds another 24 functions on the container by the time you factor in doing both for Factory and ParameterFactory.

Finally, yeah, the modifier syntax is SwiftUI-like, as are the new keyPath-style initializers. But those work with scopes, the new per-dependency decorator option, as well as a few other things that are in the pipeline.

I do appreciate the thoughts and comments.

peterfriese commented 1 year ago

Liking the new design so far. The only thing I found a bit confusing at first was the order of the parameters on ParameterFactory. I think I would've preferred listing the parameters at the end rather than at the beginning.

hmlongco commented 1 year ago

@peterfriese Liking the new design so far. The only thing I found a bit confusing at first was the order of the parameters on ParameterFactory. I think I would've preferred listing the parameters at the end rather than at the beginning.

Thanks. The order is the same as it was in 1.0, and that order matches the order in the name itself: ParameterFactory<Parameter Type, Factory Type>

And the rationale (rationalization?) is that you have to first pass in the parameter to get the dependency... ;)

doozMen commented 1 year ago

Personally I do not get why there is a need for a container and would have to see the code of 2.0 to reason about it. I'm a big fan of a simple injection system that is type safe. Only I do not see the point of Container. So what would I like to do

  1. Set dependencies
  2. reset them all
  3. set them again while app is runnning

This I do now inside a library that has its own namespace. So I figured as global variables are lazy and thread safe by default that I configure the facturies like globals and not as an extension on Container

public private(set) var typographyFactory: Factory<TypographyManager> = Factory(scope: .cached) { TypographyManager()  }

// and reset them

func bootstrap(  typography: @autoclosure @escaping () -> TypographyManager ) {
  typographyFactory.reset()
  typographyFactory.register { typography() }
}

In code I can use it then

import Factory

struct FooView: View {
@Injected(typographyFactory) private var typography
/// ...
}

I do not see why this has to change and become more complex and I'm also more worried about thread safety and lazy instantiation. Factories in my humble opinion can slow down startup time of they are all injected at once. So a system that garanties them to be lazy is my preference.

I'm now, without more knowledge of the code, strongly against adding the need for a Container. I find it refreshing to not use it. But I stand to be convinced otherwise?

Thanks for this awesome lib and the fact that we can discuss about its future! Just a tip could we not use githubs discussion thing for that instate of an issue?