Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Optional Dependencies #457

Open
dbagwell opened this issue May 11, 2023 · 3 comments
Open

Improve Optional Dependencies #457

dbagwell opened this issue May 11, 2023 · 3 comments

Comments

@dbagwell
Copy link

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 #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
Copy link
Collaborator

rudro commented May 18, 2023

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
Copy link
Author

dbagwell commented May 23, 2023

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, dbagwell#1

@sidepelican
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants