hmlongco / Resolver

Swift Ultralight Dependency Injection / Service Locator framework
MIT License
2.14k stars 188 forks source link

Safe named dependencies #83

Closed DesmanLead closed 3 years ago

DesmanLead commented 3 years ago

It would be nice to let the compiler assist with named dependencies. I propose to implement something similar to Notification.Name. So the public API will be:

extension Resolver.ServiceName {
    static let myServiceName = Self("my.service")
}

Resolver.resolve(MyService.self, name: .myServiceName)

@hmlongco what do you think about that? I'm going to implement this for our needs soon, so looking forward to your feedback.

hmlongco commented 3 years ago

So it took the PR for me to see exactly what you wanted to do and it's not a bad idea. In fact, if I'd thought of it when I created Resolver I might have done it that way.

That said, I'm somewhat disinclined to pull this into the current version of Resolver for a couple of reasons. First and foremost, it's another breaking change to the library, as anyone using service names would have to rewrite their code.

Second, strings, while not compiler safe, can come in handy from time to time. The following selects the API mode based on a string pulled from the app info dictionary. Any data-driven name would need extra code to map it to the correct namespace value.

register(DataRequesting.self) { () in
    return resolve(ClientRequesting.self, name: AppInfo.source)
}.scope(session)

Third, it's pretty trivial to retrofit a similar concept onto the current implementation if you in fact want to go this route.

enum ServiceName {
     static let alert = "alert"
 }

register(MessageListViewModel.self, name: ServiceName.alert) { () in
    MessageListViewModel<AlertMessage>(stringProvider: resolve(), messageRepository: resolve())
}

True, it's just slightly more cumbersome to type your namespace (ServiceName.alert), but from my perspective it's not overly burdensome and you get almost all of the safety and code-completion benefits without losing flexibility or breaking the existing codebase. It's true you could bypass it and hard-code strings, but doing so would be fairly obvious and tend to stand out in a code review.

Thoughts?

DesmanLead commented 3 years ago

That's right, I was concerned to introduce breaking changes. I tried adding the ServiceName on top of the current API first. But I didn't like the amount of very similar code as a result. Although it may be the way to go.

Why did I decide to introduce breaking changes instead?

1. The migration is as simple as wrapping all the name strings into the ServiceName struct:

Resolver.resolve(..., name: "test")
Resolver.resolve(..., name: ServiceName("test"))

We can make it even simpler in such cases by making SerivceName to conform ExpressibleByStringLiteral.

2. Actually, it's better to migrate non-literal usages to be ServiceName instead of a primitive type (String). McConnell would've been happy :) Considering your example:

register(DataRequesting.self) { () in
    return resolve(ClientRequesting.self, name: AppInfo.source)
}.scope(session)

Other usages of AppInfo.source are not clear here if it is a String. It may be used e.g. as a localizable string key in some other place. The new API encourages to make a separate field of type ServiceName for the dependency name. Which can be computed as well. E.g.:

struct AppInfo {
    ...
    lazy var dataServiceName: ServiceName = {
        ServiceName(self.source)
    }()
}

Why did I decide it's worth including this in the library?

For sure, it's possible to create ServiceName in the client code and just store strings there. But having SerivceName support on the Resolver level adds room for future improvements.

  1. A great thing to have is name collision handling. It's possible e.g. to collect all the names passed to ServiceName.init and check for collision in debug build.
  2. Having ServiceName in Resolver adds room for performance optimizations. I don't have anything particular in mind, but having a knowledge of all the names ever created looks viable in this context. E.g. an index could be assigned to each ServiceName on initialization to make the lookup process faster.

Does it make sense? Do you see any better ways to implement and integrate this?

hmlongco commented 3 years ago

Using ExpressibleByStringLiteral avoids breaking changes and allows both paradigms, but doesn't strictly enforce compiler safety. Hmmm. Have to mull this over for a bit.

I suppose I could introduce it but immediately mark the ExpressibleByStringLiteral version as deprecated. ;)

On another topic, if I integrate this or something similar I'd probably want to do the following instead of allowing ServiceName to pollute the global namespace.

extension Resolver {

    public struct Name: ExpressibleByStringLiteral {

        let rawValue: String

        public init(_ rawValue: String) {
            self.rawValue = rawValue
        }

        public init(stringLiteral: String) {
            self.rawValue = stringLiteral
        }

    }

}

Hence...

extension Resolver.Name {
    static let fred = Self("Fred")
    static let barney = Self("Barney")
}

Just for reference, I did the same thing when I added Args.

hmlongco commented 3 years ago

A version of this will be in 1.3. See current develop branch and documentation at:

https://github.com/hmlongco/Resolver/blob/develop/Documentation/Names.md