-
Notifications
You must be signed in to change notification settings - Fork 0
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 Task Local Inject Setup behind new option #20
base: main
Are you sure you want to change the base?
Conversation
@@ -1,4 +1,5 @@ | |||
import Foundation | |||
|
|||
public final class Container { | |||
private let localDependencyGraph: ThreadSafeDependencyGraph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove it if we are certain we want task local to be the final way (then we can even remove the options in total or make it empty for now)
|
||
|
||
public func copy() -> Self { | ||
return Self(valuesByType: self.valuesByType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would want to create a new instance of the dictionary here which takes the values in, otherwise we have the same reference to a dict under the hood which could have issues with races I think (since this is a class not a struct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are passing the dictionary into a new class, we should have a copy. Technically copy on write will make it the same instance, but it should be fine since those updates are atomic. What would be dangerous would be passing the ServiceDictionary itself, since that has a pointer to the actual same dictionary in multiple places and is definitely not sendable
@@ -0,0 +1,26 @@ | |||
enum ServiceDictionaryTaskLocal { | |||
@TaskLocal | |||
static var dictionary = ServiceDictionaryTaskLocalWrapper() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this being static would cause multiple containers to share the same local dependency graph, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is TaskLocal
it only sticks around for the given task. They all have the same base wrapper (which is an empty container), but when you go into the task (using the ServiceDictionaryTaskLocal.$dictionary.withValue
), it will update the value to be the new one. Here are the docs: https://developer.apple.com/documentation/swift/tasklocal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tasks - sure, but if you are just on the main thread and have two containers I think this could lead some funkiness.
This is kind of convoluted use case but something like this would lead to some weird behavior I think:
let containerA = Container()
let containerB = Container()
// Add modules to containers
containerA.inject { module in
module.factory {
let depB: DependencyB = containerB.inject { ... }
return depB
}
}
Is there anyway to use TaskLocal via an instance variable so it can be scoped to a container? (Also please let me know if my understanding above is incorrect).
Closes #19
Adds a way to inject values locally without locking. This allows us to do local injecting within local injecting since it will merge the two setups. All local injecting makes a copy of the currently injecting service dictionary, which allows us to have the class be sendable, since we never mutate the same underlying object.