-
Notifications
You must be signed in to change notification settings - Fork 73
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
Strategy for dependency injection (if that's the correct term) #95
Comments
At first blush, I am not a fan of these changes. Not that they are bad solutions, but that they are adding complexity for the common case. My concern is that this would be adding headache for 95% of the use cases in service of 5% of the outliers. Do you have any data on what a real fraction of users would be served by this change? One of the reasons I really love Decodable, and have used it in the last 2 commercial products I have worked on, is that it's easy to understand, simple to use, and gives me all the right information. As Apple says: When designing an API, optimize for the call site. Sometimes saying no to features makes for better software. I just re-read #29 from above and had an idea on how to at least solve that problem, not sure how feasible, but will comment in that ticket directly. |
@voidref thanks for your comments! Alternative 1 in it's current form should only require changing -static func decode(_ json: AnyObject)
+static func decode(_ json: DecodingContext<Void>)
Because `Decodable` should provide the following extensions:
in your Decoding-conforming types.
extension Decodable where Parameters == Void {
public static func decode(_ json: AnyObject) throws -> Self { ... }
}
extension Decodable {
public static func decode(_ json: AnyObject, parameters: Parameters) throws -> Self { ... }
} There are some aspects of the proposed design I realised I haven't fully thought about, it might turn out to be beneficial to implement these features with a separate protocol and new set of overloads. I would really prefer not to, but at least we're looking either at similar diff to above or no changes required for most cases. We could also do something like public protocol SimpleDecodable: Decodable {
static func decode(_ json: AnyObject) throws -> Self
}
extension SimpleDecodable {
static func decode(_ context: DecodingContext<Parameters>) throws -> Self {
return try Self.decode(context.json)
}
} But if you're manipulating
Could introduce two separate mapping-functions though.
struct DecodingContext<T> {
func mapJSON(_ closure: (AnyObject) -> AnyObject) -> DecodingContext<T>
func mapParameters<U>(_ closure: (T) -> U) -> DecodingContext<U>
} So while I don't know exactly how things could turn out or how breaking it will be, it should not be that bad, and definitely not adding any headache for the 95%.
However alternative 2 is basically change nothing. |
Injecting parameters/dependencies in sub-objects of the objects being decoded is difficult, and Decodable currently does nothing to help this.
This could be seen as the underlying problem for:
NSManagedObjectContext
(I think)Depending on the solution this could also solve/affect:
=>
has been lost in Swift 3.0)Here are two potential solutions I have thought of:
Solution 1:
DecodingContext<Parameters>
Final design would likely be more refined, for instance with more appropriate initialisers.
Pros
=>
typealias JSON = DecodingContext<NSManagedObjectContext>
if wanted.Cons
context.parameters
itself.AnyObject
which could make it more inconvenient to work with.Note:
In the above described design each object requires a context:
DecodingContext<Self.Parameters>
. For dependencies that only travel one level, e.g (the child requiring a reference to the parent) this isn't a problem. But for dependencies on the bottom (grandchildren) that needs "external" dependencies, for example the url response that the json came with, every intermediate object must also have setParameters
that include these.However, would be very weird if it didn't work that way.
Another note (added in hindsight)
Thinking more academically it would be practical if the decode protocol could require that the parameters are a subset of the
Self.Parameter
requirement.However with current swift version P cannot be constrained to an associatedtype. Thus we need separate contexts for decoding different things:
But some kind of implicit conversion (with overload) could be added for
DecodingContext<T>
->DecodingContext<Void>
conversion.Partial implementation
I have been playing around with this idea on this branch
Solution 2: Currying and no protocols
This is a method I have been (forced) to using in some places which works by not conforming to
Decodable
but instead returning a customdecode
function. Parents would then be able to inject dependencies explicitly, e.gDependencies that need to be provided at a top level have to cascade down the initialization-chain.
Pros
Decodable
Cons
=>
, which is cumbersome for objects with many children with dependencies.Decodable
to help by decoding complex types like[T?]?
is removed.## Closing notes
I think I lean more and more towards alternative 1. Nonetheless, have no strong incentive to implement this now, so going to let this sit for a while in a review-period-esque way.
So any thoughts/reactions are much appreciated!
The text was updated successfully, but these errors were encountered: