Decodable icon indicating copy to clipboard operation
Decodable copied to clipboard

Strategy for dependency injection (if that's the correct term)

Open Anviking opened this issue 8 years ago • 2 comments

class A {
    var b: B
}

class B {
    weak var a: A
    let urlResponse: String 
}

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:

  • #51 Using Decodable with Core Data – injecting a NSManagedObjectContext (I think)
  • #29 Initialising objects from different JSON sources (where passing down a format enum could be a solution)

Depending on the solution this could also solve/affect:

  • #26 (If a "DecodingContext"/wrapping type approach is taken)
  • #94 (If ability to properly chain => has been lost in Swift 3.0)

Here are two potential solutions I have thought of:

Solution 1: DecodingContext<Parameters>

public struct DecodingContext<T> {
    var path: [String]
    var json: AnyObject
    var rootObject: AnyObject
    var parameters: T

    init(json: AnyObject, path: [String] = [], rootObject: AnyObject, parameters: T) {...}

    public func parse(keys: [String]) throws -> DecodingContext {...}
    public func parseAndAcceptMissingKeys(keys: [String]) throws -> DecodingContext? {...}

    func map<U>(closure: (T) -> U) -> DecodingContext<U> {...}
}
public protocol Decodable {
    associatedtype Parameters = Void
    static func decode(_ : DecodingContext<Parameters>) throws -> Self
}
let dict: NSDictionary = ["hello": "world"]
let response = "<this is an urlresponse>"
let code = 200

let a = DecodingContext(json: dict, path: [], rootObject: dict, parameters: (urlResponse: response, code: code))
let params = a.parameters.urlResponse // <this is an urlresponse>
let params = a.parameters.code // 200

let b = a.map {
    (urlResponse: $0.urlResponse, code: $0.code, additionalParamter: true)
}
b.parameters.additionalParamter // true
extension A: Decodable {
    static func decode(_ context: DecodingContext<Void>) {
         let context = 
         return A(b: context => "b")
    }
}

Final design would likely be more refined, for instance with more appropriate initialisers.

Pros

  • DecodingContext could be a good central hub for decoding-methods, and allows for alternatives/replacements for =>
  • Easy to add dependencies for a whole tree of objects in a type safe way
  • Ability to have properties decoded according to inferred type is preserved in every instance
  • DecodingContext<Parameters> can be typealiased, e.g typealias JSON = DecodingContext<NSManagedObjectContext> if wanted.
  • Perhaps makes it more type safe.

Cons

  • Single-element-tuples cannot be created/named. This means that if you have only one parameter, you cannot give it a name, since the parameter becomes context.parameters itself.
  • Support to decode dictionaries might be difficult to pull of / inconvenient if key and value have require different parameters (unlikely though).
  • Adds a wrapping type around every 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 set Parameters 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.

static func decode<P: Parameters>(_ context: DecodingContext<P>)  throws -> Self {}

However with current swift version P cannot be constrained to an associatedtype. Thus we need separate contexts for decoding different things:

static func decode(context: DecodingContext<NSManagedObjectContext>) throws -> Self {
     let user: User = try context => "user"
     let tags: [Tags] = try context => "tags"
     // Remove the NSManagedObjectContext from the parameters
     let context = context.map { _ }
     let url = try context => url 
}

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

public static func decode(parameter: String, foo: String) -> (AnyObject) throws -> Self {
        return { json in
            try Self(json: json, parameter: parameter, foo: foo)
        }
    }

This is a method I have been (forced) to using in some places which works by not conforming to Decodable but instead returning a custom decode function. Parents would then be able to inject dependencies explicitly, e.g

try b = B(json: json => "b", a: self)

Dependencies that need to be provided at a top level have to cascade down the initialization-chain.

Pros

  • Requires no changes to Decodable
  • Very explicit and non-magical

Cons

  • Operators doesn't work on types with dependencies =>, which is cumbersome for objects with many children with dependencies.
  • Ability for 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!

Anviking avatar Jul 03 '16 20:07 Anviking

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 avatar Jul 04 '16 06:07 voidref

@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:
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 { ... }
}

in your Decoding-conforming types.

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 AnyObjects yourself it would be worse depending on how things turn out. (As you might have to wrap and unwrap it)

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%.

Not that they are bad solutions, but that they are adding complexity for the common case.

However alternative 2 is basically change nothing.

Anviking avatar Jul 04 '16 11:07 Anviking