Use the new Decodable Swift protocol to handle options

[sindresorhus]

Fixes #8

Much more readable code and we get automatic input validation in the Swift binary.

[sindresorhus]

@LarsJK Would appreciate a review here if you have time and interest. Especially the TODO comments.

[sindresorhus]

I keep wondering if it would be better to follow the Cocoa/UIKit style of optional arguments and assign after init instead.

Then we could do this instead. And we wouldn't have to repeat the default parameters each time.

recorder = try Recorder(…)

if let audioDeviceId = options.audioDeviceId {
  recorder.audioDevice = AVCaptureDevice(uniqueID: audioDeviceId)
}

I'm not really sure what the Swift best-practice is though? Maybe this way is more of a legacy Objective-C to Swift API quirk.

[LarsJK]

Yeah, IMO assign after init would be better in this case 👍

[sindresorhus]

@LarsJK I'm confused about when you should prefer to assign after init and when you should have it in the constructor. Do you know what the best practice is? For example, should all optional values be assigned after init?

[LarsJK]

Not sure what the best practice is for this, or if there is any..
The reason I feel it is better in this case is that you are duplicating the logic for the default value.

[LarsJK]

At least in Xcode 9 GM CGRect and URL conforms to Codable, so you can change that.

I think I would make a convenience initializer For Recorder that takes an Option object.

[LarsJK]

CGRect already conforms to codable the right json should be an array of two arrays of two numbers: "[[0,0],[100,100]]"

[sindresorhus]

Oh, nice. Didn't realize that. Although would be nicer if the JS side could just send an object and have it handled on the Swift side.

[LarsJK]

How about making the cropRect: CropArea private and expose a computed cropRect: CGRect property?

struct Options: Decodable {
    let destination: String
    private let cropArea: CropArea?

    var cropRect: CGRect? {
        guard let crop = cropArea else { return nil }
        return CGRect(x: crop.x, y: crop.y, width: crop.width, height: crop.height)
    }
}

There does not seem to be any way to override CGRects JSONDecoding so alternatively you would have to do something like this:

struct Options: Decodable {
    let destination: URL
    let cropRect: CGRect?

    private enum CodingKeys: String, CodingKey {
        case destination
        case cropRect
    }

    private enum RectCodingKeys: String, CodingKey {
        case x, y, width, height
    }

    public init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        destination = try container.decode(URL.self, forKey: .destination)

        let rectContainer = try container.nestedContainer(keyedBy: RectCodingKeys.self, forKey: .cropRect)
        cropRect = CGRect(
            x: try rectContainer.decode(CGFloat.self, forKey: .x),
            y: try rectContainer.decode(CGFloat.self, forKey: .y),
            width: try rectContainer.decode(CGFloat.self, forKey: .width),
            height: try rectContainer.decode(CGFloat.self, forKey: .height)
        )
    }
}

I dont think you can extend the decoding so you have to implement all of the decoding yourself..

[sindresorhus]

Too much boilerplate I would say. Easier to just change it on the JS side.

[LarsJK]

URL conforms to Codable, you should be able to just use URL instead of string

[sindresorhus]

I thought that too, but when I tried URL there I got an error:

Cannot record to URL /private/var/folders/3x/jf5977fn79jbglr7rk0tq4d00000gn/T/32e3df1e53d027351c7edbfe9db2a72f.mp4 because it is not a file URL'\n

So it seems it will only correctly coerce if you specify a file URL. I could do that on the JS side, but it feels like something I should be able to handle with a custom decodable handler or something.

[LarsJK]

Add file:// infront

struct MyClass: Codable {
    let url: URL
}

let data = "{\"url\":\"file:///private/var/folders/3x/jf5977fn79jbglr7rk0tq4d00000gn/T/32e3df1e53d027351c7edbfe9db2a72f.mp4\"}".data(using: .utf8)!

let m = try! JSONDecoder().decode(MyClass.self, from: data)

m.url.isFileURL // true

[LarsJK]

Yeah, IMO assign after init would be better in this case 👍

[LarsJK]

Is it intentional that you can no longer specify none for audioDevice? I.e. You can no longer record without audio

[sindresorhus]

Oops, good catch. Fixed now.

[sindresorhus]

Thanks for the feedback @LarsJK ✨ I think I've addressed your concerns now.

I'm gonna go ahead and merge as I need to do more changes based on this, but let me know if you see anything else that could be improved.

[sindresorhus]

I opened a new issue for improving the constructor: #50 It's not really related to this work.