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

Add @DynamicInjected property wrapper #220

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

FKREISE
Copy link

@FKREISE FKREISE commented Aug 16, 2024

Adds the @DynamicInjected property wrapper. Unlike the other property wrappers the resolved instance through the factory is not held in a stored property. This can be useful for one-shot operations where a dependency is used only in the scope of one function and then no longer needed.

Example

class MyApplication {
    @DynamicInjected(\.myService) var myService

    func doSomethingOnce() async throws {
        let result = try await myService.checkSomething()
        []
    }

    []
}

Here we don't want to keep a reference to myService because it's only needed once and we want it to deinit afterwards.

Alternatives

Computed property

Using a computed property achieves the same behavior. But the property wrapper is nicer.

class MyApplication {
    var myService: MyService {
        SharedContainer.shared.myService()
    }
}

Declaration

The program was tested solely for our own use cases, which might differ from yours.

Link to provider information

https://github.com/mercedes-benz

@hmlongco
Copy link
Owner

I'm questioning my earlier decision to allow this in the first place. Stay tuned.

@danielepantaleone
Copy link
Contributor

danielepantaleone commented Sep 5, 2024

This is actually something I was looking for and ended up in using a computed property. Any chance this will be included at some point?

@danielepantaleone
Copy link
Contributor

danielepantaleone commented Oct 10, 2024

@hmlongco Any update on this? I have been using this property wrapper (not exactly this one, but a similar version I have on my fork: danielepantaleone@14f35fe) since a while now and it's quite handy since it allows not to have computed properties around used just to resolve dependencies from a DI container. Any chance this can be included in the next release?

EDIT: Just to give some context, I have been experiencing issues when running unit tests on a swift framework I'm maintaining which makes use of Factory. When using LazyInjected or Injected in the XCTestCase extension, dependencies were being retained in the XCTestCase class extension causing unit tests to fail:

class MyTestCase: XCTestCase {
    
    @LazyInjected(\MyContainer.dataStorage) // cached
    var dataStorage
    @LazyInjected(\MyContainer.locationDao) // cached
    var locationDao

    override func setUpWithError() throws {
        try super.setUpWithError()
        MyContainer.shared.manager.reset()
        MyContainer.shared.registerMocks()
        try dataStorage.create()
    }
    
    override func tearDownWithError() throws {
        try dataStorage.erase()
        MyContainer.shared.manager.reset()
        try super.tearDownWithError()
    }
    
    ...
    
}

For example, dataStorage and locationDao are being lazily injected from my custom container, where I registered a cached factory for both. These are commonly used across the framework. In unit tests, I encountered failures when running all tests together (without parallelization), but they passed when executed individually. Switching from property wrappers to direct resolution (or computed properties) resolved the issue. This is why the DynamicProperty wrapper could be quite useful, at least in my case.

@hmlongco
Copy link
Owner

With Injected the dependencies are resolved once and then stored when the property wrapper is first instantiated. With LazyInjected, the dependencies are resolved once and then stored when the property wrapper's wrapped value is first accessed.

The biggest issue that I have with DynamicProperty is that the factory dependencies are going to be resolved each and every time the property is accessed, with a new instance returned each and every time... and that strikes me as somewhat dangerous.

In your case you're indicating that the factory value is cached and that ameliorates the issue somewhat, but the documentation for this needs to be very explicit in regard to what will occur and its drawbacks.

@danielepantaleone
Copy link
Contributor

danielepantaleone commented Oct 10, 2024

With Injected the dependencies are resolved once and then stored when the property wrapper is first instantiated. With LazyInjected, the dependencies are resolved once and then stored when the property wrapper's wrapped value is first accessed.

Yes, and they both retain a strong reference.

The biggest issue that I have with DynamicProperty is that the factory dependencies are going to be resolved each and every time the property is accessed, with a new instance returned each and every time... and that strikes me as somewhat dangerous.

Isn't this already happening if I access the factory without using the property wrapper? Indeed a new instance is created every time if unique scope is used, but no new instance is created if the scope is cached or singleton.

let dataStorage = MyContainer.shared.dataStorage() // resolved (possibly from cache)
let locationDao = MyContainer.shared.locationDao() // resolved (possibly from cache)
try dataStorage.createDatabase()
try locationDao.insert(...)

In your case you're indicating that the factory value is cached and that ameliorates the issue somewhat, but the documentation for this needs to be very explicit in regard to what will occur and its drawbacks.

No, with DynamicInjected no value is cached in the property wrapper and a new resolution is triggered every time the property is accessed, which is a shortcut to using a computed property that performs the factory resolution.

I believe, I could have used WeakLazyInjected to prevent retaining a strong reference inside my unit test class (and so to bypass the issue), but I have to somehow handle the optional unwrap and that may become tedious if the amount of references to the wrapped property is large (considering a lot of test cases)

@hmlongco
Copy link
Owner

hmlongco commented Oct 10, 2024

Isn't this already happening if I access the factory without using the property wrapper?

In your original code you were basically doing...

class MyTestCase: XCTestCase {

    lazy var dataStorage = MyContainer.shared.dataStorage
    lazy var locationDao = MyContainer.shared.locationDao

    override func setUpWithError() throws {
        try super.setUpWithError()
        MyContainer.shared.manager.reset()
        MyContainer.shared.registerMocks()
        try dataStorage.create() // NOTE: RESOLVED ONCE HERE
    }
    ...
}

So dataStorage and locationDao would have been resolved once per instance of MyTestCase, at the point in time when first accessed.

What DynamicInjected would do is...

class MyTestCase: XCTestCase {

    var dataStorage: DataStorage { MyContainer.shared.dataStorage }
    var locationDao: LocationDao { MyContainer.shared.locationDao }

    override func setUpWithError() throws {
        try super.setUpWithError()
        MyContainer.shared.manager.reset()
        MyContainer.shared.registerMocks()
        try dataStorage.create() // NEW INSTANCE RESOLVED HERE
    }

    override func tearDownWithError() throws {
        try dataStorage.erase() // CACHED INSTANCE RESOLVED HERE
        MyContainer.shared.manager.reset()
        try super.tearDownWithError()
    }
    ...
}

Assuming...

extension MyContainer {
   var dataStorage:  Factory< DataStorage> { self { DateStorer() }.cached }
}

If the Factory wasn't cached, then the setup and teardown code would grab a new instance each and every time.

@danielepantaleone
Copy link
Contributor

Yes, and that's exactly what I'm looking for.

I understand that if the Factory is not cached a new instance is generated every time the property is accessed and probably that may be a little bit confusing, but that's what will happen with a computed property anyway. Maybe a warning could be printed when DynamicInjected is used on a non cached Factory so that the user will notice the "issue" (assuming is not intentional).

I can live with the computed property though, but I believe this could be a good addition to Factory.

@hmlongco hmlongco merged commit dbed962 into hmlongco:main Oct 10, 2024
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

Successfully merging this pull request may close these issues.

3 participants