uber / needle

Compile-time safe Swift dependency injection framework
Apache License 2.0
1.84k stars 144 forks source link

Improve Optional Dependencies #457

Open dbagwell opened 1 year ago

dbagwell commented 1 year ago

It would be nice if the framework allowed for lower scope to have an optional property provided by an ancestor scope with a non-optional of the same type. Like was once mentioned in https://github.com/uber/needle/issues/66 .

In addition it would be great if optional dependencies didn't have to be satisfied by an ancestor scope. Where if an optional dependency isn't provided by an ancestor it just returns nil.

This would allow for something like this:

final class LoggedOutComponent: BootstrapComponent {

    func loggedInComponent(userName: String) -> LoggedInComponent {
        return LoggedInComponent(userName: userName, parent: self)
    }

    var gameComponent: GameComponent {
        return GameComponent(parent: self)
    }

}

final class LoggedInComponent: Component<EmptyDependency> {

    let userName: String

    init(userName: String, parent: Scope) {
        self.userName = userName
        super.init(parent: parent)
    }

    var gameComponent: GameComponent {
        return GameComponent(parent: self)
    }

}

protocol GameDependency: Dependency {
    var userName: String? { get }
}

final class GameComponent: Component<GameDependency> {}

In this example, GameComponent has an optional dependency of userName that would be provided by LoggedInComponent when created from that scope, but would be nil when created from the LoggedOutComponent scope since there is no provider.

rudro commented 1 year ago

You can make this work today by just putting an optional String userName in both the logged in and logged out components. So in logged out you simply have

var userName: String? { return nil }

In the logged in component, it get's a slight bit awkward because you have to make userName optional even though you know it's a non-optional string. You can store it locally as String with another name and then have a

var userName: String? { return nonOptionalUserName }

When I teach the needle course to new eng at Uber, the example we give is pretty much exactly this, we point out that there is some dependency that only exists when you are logged in, but not logged out. In those cases, only children of logged in can get some dependency (say some auth token), if you tried to get at an auth token on the logged out side you just get an error from needle and realize that the dependency simply doesn't exist or make sense in the logged out situation.

I guess in you case I would recommend that descendants of logged out should not be asking for the user name.

dbagwell commented 1 year ago

I understand the workaround you mentioned, but feel it kind of defeats the purpose of using dependency injection in the first place. Having to declare an extra property for the non-optional is awkward like you said and would be better if it could be avoided. Also, the logged out component has no business declaring a user name property if it will always be nil.

I guess in you case I would recommend that descendants of logged out should not be asking for the user name.

This is a generalized example, but the idea is that GameComponent is accessible to both logged in and logged out users and has a lot of logic that is the same regardless of whether the user is logged in or not with a single difference of showing the user's name if they are logged in. So it is entirely appropriate that this descendant of logged out be asking for for the user name.

FWIW, I've implemented my suggested improvements on a fork, https://github.com/dbagwell/needle/pull/1

sidepelican commented 1 year ago

I believe making the definition of dependence implicitly optional might potentially lead to issues.