Shank icon indicating copy to clipboard operation
Shank copied to clipboard

Requesting an item that's injected creates a new instance every time

Open kerrmarin opened this issue 6 years ago • 5 comments

Consider the following:

protocol Service {
    var name: String { get }
}

final class HTTPService: Service {
    let name = "HTTPService"

    deinit {
        print("Deinit")
    }
}

protocol ViewModelType {
    var name: String { get }
}

final class ViewModel: ViewModelType {

    @Inject private var service: Service

    var name: String {
        print(self.service.name)
        return "Test dependency injection \(self.service.name)"
    }
}

And a dependency with the following modules:

    static let dependencyResolver = Dependencies {
        Module { HTTPService() as Service }
    }

If you ask for the viewmodel's name you'll see that the service is created each time you request it (i.e. the module's closure is executed each time you request the dependency). In this example, you'll see "deinit" printed twice.

I believe this should not be the case.

kerrmarin avatar Oct 08 '19 08:10 kerrmarin

One way to solve this would be to make the property wrapper a class and:

@propertyWrapper
public final class Inject<Value> {
    private let name: String?
    private var value: Value?
    
    public var wrappedValue: Value {
        if let value = self.value {
            return value
        }
        let value: Value = Dependencies.root.resolve(for: name)
        self.value = value
        return value
    }
    
    public init() {
        self.name = nil
    }
    
    public init(_ name: String) {
        self.name = name
    }
}

Of course this would create a new instance per @Inject, so other improvements could be made to request either a single instance always (i.e. "inject a singleton") or a different instance per @Inject Happy to make a PR

kerrmarin avatar Oct 08 '19 09:10 kerrmarin

Good catch @kerrmarin!

I was expecting that new instances would be created per property, but didn't realize it was creating a new instance per access. I fixed it in 91b5841153d14af3cba0fe2024a2729b869a89ef.

Regarding singletons, I felt the creator of the class can create a .shared static property as needed instead of burdening the DI implementation:

final class HTTPService: Service {
    static let shared = HTTPService()
}
...
let dependencyResolver = Dependencies {
    Module { HTTPService.shared as Service }
}

Thanks for contributing!

basememara avatar Oct 16 '19 18:10 basememara

@basememara It's probably worth documenting this behaviour.

I just got caught with some low-level Repos being re-created each time, and they take in properties - so there is no trivial singleton to be had.

sureshjoshi avatar Feb 17 '20 23:02 sureshjoshi

That’s a really good use case, hadn’t thought of that. But if different properties are fed into it, would a second instance be stored? I’d imagine the properties would have to be part of the key name. I’ll give it more thought, but will get it documented in the meantime and leave this ticket open. Thanks for the suggestions.

basememara avatar Feb 18 '20 00:02 basememara

I'll be the first to say I'm using the lib wrong, but here's what I did (for example). RealmStore was fine, accountRepo was continually re-created.

I left the deps as static here, but even when they're just instances hanging off AppDelegate, the same thing appears to happen.

    private static let dependencies = Dependencies {
        Module { AccountRepository(memory: MemoryAccountDataSource() as AccountDataSource }
        Module { RealmStore.shared as RealmStore }
    }

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
        // Override point for customization after application launch.
        Self.dependencies.build()
        ...

sureshjoshi avatar Feb 18 '20 03:02 sureshjoshi