hmlongco / Factory

A new approach to Container-Based Dependency Injection for Swift and SwiftUI.
MIT License
1.71k stars 107 forks source link

Add resolve context to overrule the context at resolution time #106

Closed doozMen closed 1 year ago

doozMen commented 1 year ago

For my project it is very important that there is one global context that all registrations use and a resolve context that I can use to force a resolution to take a factory context into account. I hope we can at least discuss the option of adding this feature. Maybe the implementation is not as you would like it to be in Factory but could you point me into a direction or should I remain on a fork?

I tried to prepare for this merge request by first refactoring the way a context was used to look for the appropriate factory in #105 but that request was closed and I was pointed to develop. I removed the need to refactor the code but just add the feature.

The added unit testsFactoryContextTests.swift:294 testResolveInContext() should clarify how it can be used.

Question: I also notice that develop does not seam to be up to date with main. So unclear if I should point this to main or develop?

hmlongco commented 1 year ago

Is this for testing? (i.e. force a context)? If so it might simply be easier to make the FactoryContext static variables public. If you look at the unit tests for contexts you'll see how I manipulate the context during testing to exercise all of the paths.

    func testWithArgumenst() {
        FactoryContext.isPreview = false
        FactoryContext.isTest = false
        FactoryContext.arguments = ["ARG2"]
        let service1 = Container.shared.externalContextService()
        XCTAssertEqual(service1.name, "REGISTERED")
        let service2 = Container.shared.internalContextService()
        XCTAssertEqual(service2.name, "DEBUG")
        let service3 = Container.shared.argsContextService()
        XCTAssertEqual(service3.name, "ARG")
    }
hmlongco commented 1 year ago

Also think this may be be what Runtime Arguments was designed to handle.

FactoryContext.setArg("light", forKey: "theme")
FactoryContext.removeArg(forKey: "theme")

myStyleSystem { StandardTheme() }
    .onArg("light") { LightTheme() }
    .onArg("dark") { DarkTheme() }

FactoryContext args are global across all containers.

hmlongco commented 1 year ago

Download the current develop branch and check out FactoryContext.current. This should allow global overrides for any of the context states.

doozMen commented 1 year ago

Yes I tried exposing that but as it is used in a multithreaded way I could not use it to overwrite temporarily. Mainly putting back the context that it was before was problematic. The fact that it is global makes it hard to reason with and difficult as you would need to

  1. store the current arguments
  2. put the current arguments back

But what if inbetween 1 and 2 the current arguments change, on another thread. Then you would be putting arguments back in 2 that are not the most recent.

So instead of doing that I thought as I only need it when I do, what I call manual, resolution, not when jusing @Injectec(..) or Container.shared.foo() this might be cleaner as I could use in code where I need it Container.shared.foo.resolve(in: ...) which does not care about the global arguments, but returns the value for the context I enforce.

So in short you are very correct and I tried to use runtimeArguments, then name indicates they are to be used for that use case. But I had trouble in a multi threaded world, which all of my projects are :).

hmlongco commented 1 year ago

You mean something like...

extension Container {
    enum Condition {
        case a
        case b
        case c
    }
    var conditionalInjection: ParameterFactory<Condition, MyServiceType> {
        ParameterFactory(self) { condition in
            switch condition {
            case .a:
                return MyService()
            case .b:
                return MockService()
            case .c:
                return ValueService()
            }
        }
    }
    func test() {
        let service = Container.shared.conditionalInjection(.b)
    }
}

As to contexts, they are really meant to be global and permanent throughout the app's lifetime. You don't generally switch from running in a unit test in a simulator to running prod on a device. IOW, it's the context in which the application is running.

doozMen commented 1 year ago

I would like to keep using @injected an as I understand it the ParameterFactory cannot be used, for good reason, with that?

However that is not my main issue. The registrations in my project come from different libraries. Depending in which lib is added as a dependency there are more or less .onArg... registrations. Making for me the use of this context feature ideal as it offers me a way to define a dynamic context without writing that myself. I do however need to compose some resolutions of both light and dark themes as you noticed. So I got this working without the use of a parameterfactory which I found an advantage. Hence I do not think, however I love the suggestion, that using a parameter factory would make it possible accomplish my goal.

So what do you need to see changed in order for this to be approvable?

hmlongco commented 1 year ago

If you want to use @Injected then all I can think of for now is....

extension Container: AutoRegistering {
    var dynamicTheme: Factory<Theme> {
        self {  self.lightTheme() }
            .onArg("light") {  self.lightTheme() }
            .onArg("dark") { self.darkTheme()  }
    }
    var lightTheme: Factory<Theme> {
        self { LightTheme()  }
    }
    var darkTheme: Factory<Theme> {
        self { DarkTheme()  }
    }
}

Places that want dynamicTheme inject it. Places that need to force light or dark mode inject those directly.

You can't use parameter factory with injected as there's no way to pass dynamic parameters.

hmlongco commented 1 year ago

If no more comments on this I'll close it?