Closed ZsoltMolnarrr closed 2 years ago
You did a pretty thorough writeup, but I'm still not exactly sure what problem it is you're trying to solve. As such, I'm not convinced this is the best solution to the problem.
Couple of things. Keep in mind that the parameter to .container doesn't have to a single static var. e.g.
extension Resolver {
static func myFeatureServices() -> Resolver {
let container = Resolver(parent: .main)
container.register { MyFeatureService() }
return container
}
}
class MyFeatureService { }
class MyFeature {
@Injected(container: .myFeatureServices()) var service: MyFeatureService
}
Also not sure if helpful, but if you use @LazyInjected you can access and swap out the container before resolution occurs.
Hello @hmlongco! I am sorry for the delayed response. I would like continue discussion of the topic.
Let me describe the problem in a different way. We want to manage our containers dynamicly, it means the following:
There is no conflict in resolving until services inside the container have the following scopes: .shared
, .transient
.
But for many of our services we want their lifecycle to be the same as the container (in other words, singleton within the container). Since resolver has no .container
scope we can use a custom ResolverScopeCache
instance, that we make sure to have matching lifecycle as the container.
(Side note: I would like to create a separate PR to introduce the .container
scope.)
Currently we cannot have multiple modules with identically assembled containers, containing services with .container
scope and use propertyWrapper injection at the same time. (There is no way to make sure the propertyWrapper injection will use the correct, module specific scopeCache.)
I know it sounds like an edge case, but we love propertyWrapper injection, and we cannot work with static containers.
(Assembling = performing registrations on a container) (Module = a semantical high level component that manages a featureset, not an actual Swift framework)
There's a chicken and egg aspect to proposed solution and in the demo code that I still don't understand. (And it's entirely possible I'm being dense.) But doing...
class MyFeature {
@Injected(container: .context) var service: MyFeatureService
}
Basically says "look in the current context variable for the actual container to use." Fine. But what set the current context to the proper container?
The PR code shows the registration block factory setting the context based on the container captured during registration. Okay... that's fine to set things up for any sub-dependencies... but that factory for MyFeatureService
has to be found first, before its registration block can be called and before it has a chance to swap out the context.
So again, who set the current context to the proper container before the injection occurred, so it could find the proper registration and factory?
I think this is close to doing what you want...
protocol ContainerOwner: AnyObject {
var container: Resolver? { get set }
}
extension Resolver {
static func dynamicRegistrationBlock<Feature:ContainerOwner>(factory: @escaping (_ container: Resolver) -> Feature) -> Feature {
let current = Resolver.root
Resolver.root = Resolver(parent: current)
defer { Resolver.root = current }
let feature = factory(Resolver.root)
feature.container = Resolver.root
return feature
}
@discardableResult
func dynamicRegistration<Service>(factory: @escaping () -> Service) -> ResolverOptions<Service> {
register(Service.self) { [unowned self] in
let current = Resolver.root
Resolver.root = self
defer { Resolver.root = current }
return factory()
}
}
}
class MyFeatureServiceA {
@Injected var service: MyFeatureServiceB
}
class MyFeatureServiceB {
@Injected var service: MyFeatureServiceC
}
class MyFeatureServiceC {
}
class MyFeature: ContainerOwner {
static func factory() -> MyFeature {
Resolver.dynamicRegistrationBlock { container in
container.dynamicRegistration { MyFeatureServiceA() }
container.dynamicRegistration { MyFeatureServiceB() }
container.dynamicRegistration { MyFeatureServiceC() }
return MyFeature()
}
}
@Injected var service: MyFeatureServiceA
var container: Resolver?
}
The key is calling MyFeature.factory()
to get an instance of MyFeature
, as that function is the "gateway" into the model, as well as the registration block for it.
It creates a new, dedicated container, registers factories into it, and sets up a new root before instantiating MyFeature
. Sub-dependencies also set/reset root to that container before calling the associated factory.
Note that MyFeature
MUST store the container, as each one is scoped to that particular instance, and is necessary given the aforementioned requirements.
Given those requirements, I think this is as clean as you're going to get.
Hello! Thank you very much for the continued investigation!
Yes this is very close to what we want to achieve. The example code you gave about how a feature is made up very close to our goal. We kept iterating over the idea, then the final came to be the code of this PR. Let me explain our choices.
About the propertyWrapper injection: We preffer this:
class MyFeatureServiceA {
@Injected(container: .context) var service: MyFeatureServiceB
}
over this:
class MyFeatureServiceA {
@Injected var service: MyFeatureServiceB
}
When a project uses the non static container approach, we wanted to make sure the property declaration makes it clear where the injected comes from. When (container: .context)
is not present, it can be difficult to figure out if it is injected from the root container, or from a contextual one.
About the registration: Our preferred registration method has the following header:
func assemble(container: Resolver)
This takes the container as an argument, because we expect MyFeatureA
to create MyFeatureB
and pass a new child of A's container to B.
Instead of creating custom registration functions we thought that this new registration mutator, would be more idiomatic for the API.
Hence we put the setContainer; { defer unsetContainer
logic (what your example includes in the custom registration functions) into this new mutator.
At this point I still stand by our original idea. Sure it would be easy for us to just create an extension and solve it there, the only tradeoff would be the usage of custom registration functions which is not as nice as the mutator. But since I encountered this specific problem on multiple projects, I think this feature could be an actual selling point of this library, and would help many others in the same boat (if backed by good documentation). I acknowledge that, the issue you brought up about recursive resolution really is a problem, and should be addressed. (For example with the usage of a Stack).
What do you think about the original idea? Is this something, that goes against the semantics of this library? Should I make more example code to demonstrate how exactly we want to develop features and leverage the power of the proposed feature?
I have been looking for a solution to easily resolve from dynamic containers. Such an idea could be a solution to this problem.
@hmlongco I did not address your following question directly: "So again, who set the current context to the proper container before the injection occurred, so it could find the proper registration and factory?"
Looks like the comment that followed you discovered the answer for that.
Just to be sure, let me share how I see it.
To make sure the Resolver.context
property has the correct value we need to do the following:
(Ignoring the issue of recursive resolution, to simplify the conversation.)
To implement this, there are multiple approaches
A) No custom function
Just include the code in the factory closure passed the register
function.
This results in lot of code duplication.
Example:
container.register {
Resolver.tempContext = container
defer { Resolver.tempContext = nil }
return MyServiceA()
}
B) Custom registration function This technically works, but we dislike that in case of mixed usage of this and normal registration function, the registration code is more difficult to read. Example (based on your comment):
extension Resolver {
@discardableResult
func dynamicRegistration<Service>(factory: @escaping () -> Service) -> ResolverOptions<Service> {
register(Service.self) { [unowned self] in
let current = Resolver. tempContext
Resolver. tempContext = self
defer { Resolver. tempContext = current }
return factory()
}
}
}
C) Add registration mutator
This does the same as the option B), but the way it achieves it is by changing the registration after its creation. The factory
property is simply wrapped to achieve the same thing.
public struct ResolverOptions<Service> {
// ...
@discardableResult
public func context(_ resolver: Resolver) -> ResolverOptions<Service> {
let exisitingFactory = registration.factory
registration.factory = { [weak resolver] r, a in
Resolver.tempContext = resolver
defer { Resolver.tempContext = nil }
return exisitingFactory(r,a)
}
return self
}
// ...
}
As mentioned before, we think the most idiomatic for the API would be option C).
Sorry for the delay, but I have a day job (grin). I did, however, spend some more time on this and decided that I have to decline this particular PR.
A library author has to balance need, direction, usability, and a host of other things when evaluating PRs with new features, and this new operator, to me, requires a lot of moving parts that have to be setup correctly outside of Resolver in order for the operator to be understood and used correctly.
I also think that a version of the code I posted earlier (and updated below) is the better approach.
All that said, if you checkout the current master branch you'll see that I updated Resolver such that you should now be able to easily add the feature itself, should you want to do so.
I considered simply making a bunch of the properties public, but rejected that approach as it would tie me to the current implementation and could prevent me from making other improvements and behind-the scene changes in the future.
After some reflection, I decided to add an update(factory:)
function to the registration which basically lets you wrap an existing registration factory with your own code. This opens the door to other user-defined functionality and also enabled me to cleanup the implementation for resolveProperties
, which in turn let me streamline the resolution code for almost every other resolved service.
A win-win.
Here's a sample.
public extension ResolverOptions {
@discardableResult
func context(_ container: Resolver) -> ResolverOptions<Service> {
registration.update { existingFactory in
return { [weak container] (resolver, args) in
Resolver.tempContext = container
return existingFactory(resolver, args)
}
}
return self
}
}
Download the master branch and play with this and see if it behaves as expected.
Updated version of earlier code enabling .context in injection property wrappers. I get your dislike of the different syntax for context registrations, but the fact remains that they ARE different in that they require the context to be setup correctly prior to resolution.
protocol ResolverContextOwner: AnyObject {
var container: Resolver? { get set }
}
extension Resolver {
static var context: Resolver!
static func contextRegistrationFactory<Feature:ResolverContextOwner>(factory: @escaping (_ container: Resolver) -> Feature) -> Feature {
Resolver.context = Resolver(parent: root)
let feature = factory(Resolver.context)
feature.container = Resolver.context
return feature
}
@discardableResult
func contextRegistration<Service>(factory: @escaping () -> Service) -> ResolverOptions<Service> {
register(Service.self) { [unowned self] in
Resolver.context = self
return factory()
}
}
}
class MyFeatureServiceA {
@Injected(container: .context) var service: MyFeatureServiceB
}
class MyFeatureServiceB {
@Injected(container: .context) var service: MyFeatureServiceC
}
class MyFeatureServiceC {
}
class MyFeature: ResolverContextOwner {
static func factory() -> MyFeature {
Resolver.contextRegistrationFactory { container in
container.contextRegistration { MyFeatureServiceA() }
container.contextRegistration { MyFeatureServiceB() }
container.contextRegistration { MyFeatureServiceC() }
return MyFeature()
}
}
@Injected(container: .context) var service: MyFeatureServiceA
var container: Resolver?
}
Hello! Thank you for the thorough investigation and detailed answer. Also thank you for the changes, giving us the option use this as an extension. We will continue to do so. I will keep thinking about this problem, to see if a cleaner solution comes up.
Add
context
registration mutatorIntroduction
Introducing the new
context
registration mutator function inResolverOptions
. This mutator allows the@*Injected
propertyWrappers to resolve from non-static, user defined container.Motivation
The usage of injection property wrappers is a convenient and safe way of using the framework. Unfortunately there is a compile time issue when the user tries to pass a non-static container to be used for resolution.
Resolver conveniently avoids this topic by suggesting to use staticly declared containers. For example the following works nicely:
Static containers work fine for some projects, but not for others. For example: running more than 1 instance of a feature might cause problems in more complex cases.
A clean solution is to use non static containers.
For example:
Although this approach is totally functional, produces a lot of boilerplate code (large initialisers).
The goal is to combine the clean approach of non-static containers, and the convenience of propertyWrapper injection.
Proposed solution
Create a new registration mutator function, that takes a container as its argument. When applied to a registration, the dependencies of the resolved service can access the given container via a static getter that is dedicated for this purpose.
Example usage:
The static container named
Resolver.context
is read only for the user, references another static container that is not exposed for the user and temporary has the correct value (during resolution).While it can be said that it is not very elegant when we look under the hood, looking it from outside feels easy and intuitive to use.
Effect on API
The suggested solution is small and backward compatible.
Alternatives considered
ProperyWrappers to access a container instance from
_enclosingSelf
. This has several issues, such as:_enclosingSelf
keyword