Swinject / SwinjectAutoregistration

Swinject extension to automatically register your services
MIT License
251 stars 45 forks source link

Mix dependencies with and without arguments #55

Closed Michael-Kr closed 4 years ago

Michael-Kr commented 4 years ago

Hi! Firstly I want to say thank you for your job. SwinjectAutoregistration is a really helpful tool.

The issue I've encountered is that when an object has an argument and several dependencies which may have or not have the same argument the autoregistration does not work because it does not pass the argument of the first object down to its dependencies.

// First class has an argument and 2 dependencies 
// where B also has the argument, but C has not.
class A {
    let b: B
    let c: C
    let argument: String

    init(argument: String, b: B, c: C) {
        self.b = b
        self.c = c
        self.argument = argument
    }

    func report() {
        print("I am A with \(argument)")

        b.report()
        c.report()
    }
}

class B {
    let argument: String

    init(argument: String) {
        self.argument = argument
    }

    func report() {
        print("I am B with \(argument)")
    }
}

class C {
    func report() {
        print("I am C")
    }
}

// And the usage
let container = Container()

container.autoregister(A.self, argument: String.self, initializer: A.init)

container.autoregister(B.self, argument: String.self, initializer: B.init)

container.autoregister(C.self, initializer: C.init)

// will fail in current implementation
container.resolve(A.self, argument: "1")?.report()

I've figured out that if your Resolver extension and auto-registration code will be updated to try resolutions with arguments like this: (Obviously it should be applied to all the combinations)

func resolve<Service, Arg1>(argument  arg1: Arg1) -> Service? {
   return (arg1 as? Service) ?? self.resolve(Service.self) ?? self.resolve(Service.self, argument: arg1)
}

@discardableResult
func autoregister<Service, A, B, C, Arg1>(_ service: Service.Type, name: String? = nil, argument  arg1: Arg1.Type, initializer: @escaping ((A, B, C)) -> Service) -> ServiceEntry<Service> {
   return self.register(service.self, name: name, factory: { res, arg1 in 
           let a: A? = res.resolve() ?? res.resolve(argument: arg1)
           let b: B? = res.resolve() ?? res.resolve(argument: arg1)
           let c: C? = res.resolve() ?? res.resolve(argument: arg1)
           checkResolved(initializer: initializer, services: a, b, c)
           return initializer((a!, b!, c!))

   } as (Resolver, Arg1) -> Service)
}

Than the auto-registration will work fine except that Swinject will write some scary logs about failed resolutions.

Do you know how to deal with the resolution failed logs? Should we deal with them or leave to warn developers in case the arguments passing was not expected?

What do you think about such an approach overall? I can make a PR if you think it is the right way to go.

tkohout commented 4 years ago

Hello @Michael-Kr , thanks for taking interest in the project! There's a few things that comes to mind:

  1. We have to be very careful not to cause breaking changes in library users' existing code. Even though the resolve(argument:) is applied only after the simple resolve returns nil, that still might be a valid result as we now support registration of optional services. It would certainly be edge case but we should not ignore it.

  2. Even if it doesn't cause conflicts, for every optional service that returns nil we would now have to try up to 3 arguments, making the autoregistration slightly less efficient

  3. But more that I think about it the main problem would be in the simple combinatorics of applying this to up to 3 arguments. If you for example pass 3 arguments a,b,c and then one of the nested dependencies needs argument a and the second nested dependency needs a,b,c and third maybe ca we would have to try to resolve all permutations of arguments so a, b, c, ab, ba, ac, ca, bc, cb, abc, acb ... well you get the idea.

For your use case - if you want to get rid of the logs, have a look at Container.loggingFunction