Swinject: API for resolving dependencies with arguments is typo-prone

[jakubvano]

After it took us a while of staring at the code to figure out the source of bug, we've identified a place for possible improvement in Swinject API:

Consider following lines:

container.resolve(SomeType.self, argument:(a, b, c))
container.resolve(SomeType.self, arguments:(a, b, c))

The difference in syntax is IMO much more difficult to spot than it should be, and main problem is that both lines are perfectly valid, except former most often does not capture the intent.

Some alternative syntaxes that come to mind:

// change the param name
container.resolve(SomeType.self, multipleArguments: (a, b, c)) 

// use variadic params
container.resolve(SomeType.self, arguments: a, b, c)`

Thoughts?

[yoichitgy]

@jakubvano thank you for submitting the issue. It's a good idea to improve Swinject API to provide an easy way to detect mistakes.

Basically I would like to enhance the API as you told, but labelling multipleArguments looks still a preference of each Swinject user because of its length. Some like the syntax, but the others not.

If Swift accepts varargs with different types, the latter idea using variadic params looks better.

I would like to find the best way before we make a breaking change.

[yoichitgy]

Another idea is producing a compile error if a tuple is passed to argument though I'm not sure whether this is possible.

[yoichitgy]

Yet another idea is shortening argument parameter to arg, though arg and arguments have inconsistency.

container.resolve(SomeType.self, arg:(a, b, c))
container.resolve(SomeType.self, arguments:(a, b, c))

[jakubvano]

If Swift accepts varargs with different types, the latter idea using variadic params looks better.

You are right, variadic parameters probably can't be used here.

What about

resolve<Service, Arg1, Arg2, Arg3>(serviceType: Service.Type, 
    arguments arg1: Arg1, _ arg2: Arg2, _ arg3: Arg3) -> Service? {
    ...
}

Then it could be used almost like variadic params:

container.resolve(SomeType.self, arguments: a, b, c)

[yoichitgy]

That looks better. Let me check how Xcode autocompletes the syntax.

[yoichitgy]

@jakubvano, I checked the autocompletion of Xcode with the syntax, and it worked well. If we decide to change the API, we'll use the API you proposed.

Personally I like the original API because it doesn't use the irregular external parameter name at the first parameter, but I understand your motivation to change and enhance the API😃 My opinion might be just for beauty from the view of the framework author, and might lack the view of the framework users😉

Since this is a breaking change and not an urgent issue, I would like to gather more opinions here and around me locally. Then we'll decide whether we'll make the breaking change. Thanks again @jakubvano for your idea❗️

[yoichitgy]

Please anyone feel free to leave a comment to tell your preference🙏 Which do you prefer, the current API or the proposed API❓

Usage:

// Current API
container.resolve(SomeType.self, argument: a)
container.resolve(SomeType.self, arguments: (a, b, c))

// The proposed API
container.resolve(SomeType.self, argument: a)
container.resolve(SomeType.self, arguments: a, b, c)

The definitions of API to pass multiple arguments:

// Current API
func resolve<Service, Arg1, Arg2, Arg3>(serviceType: Service.Type,
    arguments: (Arg1, Arg2, Arg3)) -> Service?

// The proposed API
fun resolve<Service, Arg1, Arg2, Arg3>(serviceType: Service.Type, 
    arguments arg1: Arg1, _ arg2: Arg2, _ arg3: Arg3) -> Service?

[yoichitgy]

@jakubvano, for now, without a breaking change, I added #65 for easier debugging. Thank you for noticing me about debugging with Swinject😃

[jakubvano]

Cool, easier debugging will definitely be useful. I agree, api syntax should be carefully considered before introducing breaking changes - I'm glad to see progress in the matter.

[yoichitgy]

Thanks for your understanding!

[thalmicMark]

I'd prefer the proposed API. It's much easier to understand what's going on in these two cases:

container.resolve(SomeType.self, argument: (a, b, c))
container.resolve(SomeType.self, arguments: a, b, c)

[yoichitgy]

@thalmicMark thanks for your opinion👍

[juancazalla]

👍 to the proposed API

[Lutzifer]

https://github.com/Swinject/Swinject-CodeGen just got released and might be of help regarding this issue.

[tomassliz]

@Lutzifer I don't think that this solves this "issue". It's only moved from your own code to the generator code. So it will be nice to approve the proposed API.

[Lutzifer]

@tomassliz I only said "might be of help", not "completely solve" 😉

Defining

SomeType;SomeType;;a:String;b:String;c:String
SomeType;SomeType;;tuple:(String, String, String)

produces this generated code (registration code left out for brevity):

import Swinject

extension Resolvable {

    func resolveSomeType(a a: String, b: String, c: String) -> SomeType {
        return self.resolve(SomeType.self, arguments: (a, b, c))!
    }

    func resolveSomeType(tuple tuple: (String, String, String)) -> SomeType {
        return self.resolve(SomeType.self, argument: tuple)!
    }
}

(...)

which actually should avoid the original problem, as the code becomes

container.resolveSomeType(a:a, b:b, c:c)
container.resolveSomeType(tuple:(a,b,c))

which are quite different calls. Also, typo-proneness in generated code is somewhat negligible, as nobody types it 😉

+1 for improving the API at this point, though

[tomassliz]

@Lutzifer I know, I don't say that whole generator thing is bad idea. I say that you only hides the bad/missleading Swinject syntax behind the code produced by the generator. But I get your point and thanks for another option how to use Swinject.

[yoichitgy]

@tomassliz, @Lutzifer: thanks for the good discussion👍 I'm preparing Swinject v2 with some breaking changes. I'll fix this issue in v2. Probably I'll take the proposed API but still thinking. I'll fix the other issues first to take time to think about the API.

[yoichitgy]

Let's migrate the API as @jakubvano mentioned in his comment.

[yoichitgy]

Resolved by #118. Thanks @jakubvano and everyone commented here!