BottleRocketStudios / iOS-Hyperspace

An extremely lightweight wrapper around URLSession to make working with APIs a breeze.
Apache License 2.0
47 stars 17 forks source link

Feature/async #160

Closed wmcginty closed 1 year ago

wmcginty commented 2 years ago

I'd like to get feedback on what a potential structured concurrency version of Hyperspace could look like. Please keep in mind this is still in draft, so nothing is anywhere near final, documentation isn't updated and there is plenty of testing / iteration still needed. The high level changes so far are as follows:

Any early feedback, requests or thoughts are greatly appreciated.

GrandLarseny commented 2 years ago

Moving from closures to async/await is a pretty major shift. Should we make this a major version update?

wmcginty commented 2 years ago

Yea, that's definitely the plan - it'll get merged (hopefully) and then when we set out for a release it'll be a new major release.

br-tyler-milner commented 1 year ago

I haven't done an in-depth review yet, but I have some initial thoughts about < iOS 15 support. Although iOS 16 is right around the corner, it feels like only projects starting in the next couple of weeks would really be able to take advantage of it. Any current projects/SOW's are probably locked to iOS 14 for a little while longer. Would it be better to have an "async" feature branch that we maintain for a little while? Or maybe there's a way we could provide a subspec for a client-friendly async interface without needing to refactor all of the inner workings at this time? Also, if I'm not mistaken, didn't Xcode 13 introduce backward compatibility for async/await all the way back to iOS 13? Is there a reason why we can't make use of that to keep the deployment target bump to iOS 13+?

wmcginty commented 1 year ago

@br-tyler-milner The general async / await functionality was backported but the URLSession functionality in iOS 15 was not. But, I can take a look to see if I can manually convert the old completion-handler based functions into an async / await paradigm for those people who are not on 15+ and mark the new methods as @available(*, iOS 15.0)

Let me see what I can do.

wmcginty commented 1 year ago

@br-tyler-milner @GrandLarseny

I just pushed a possible way we could approach this, by basically building a parallel path for < 15 users.

public protocol BackendServicing {
     ...

    // Deprecated in iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0
    func execute<R>(request: Request<R>) async throws -> R

    @available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
    func execute<R>(request: Request<R>, delegate: TransportTaskDelegate?) async throws -> R
}
public protocol Transporting {

    // Deprecated in iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0
    func execute(request: URLRequest) async throws -> TransportSuccess

    @available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
    func execute(request: URLRequest, delegate: TransportTaskDelegate?) async throws -> TransportSuccess
}
public protocol TransportSession {
    ...

    // Deprecated in iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0
    func data(for request: URLRequest) async throws -> (Data, URLResponse)

    @available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
    func data(for request: URLRequest, delegate: TransportTaskDelegate?) async throws -> (Data, URLResponse)
}

This allows clients on <15 to call backendService.execute(request: myRequest) and clients on 15 + to call backendService.execute(request: myRequest, delegate: delegate).

The downside is that it currently requires pretty much complete duplication for the main code path in TransportService, BackendService and URLSessions conformance to TransportSession. There may be some small cleanup that can be done here, but I'd have to take a closer look.

The other alternatives that I can think of:

GrandLarseny commented 1 year ago

@wmcginty I really like this solution, seems exactly what @available checks were intended for. Two thumbs up from me!

wmcginty commented 1 year ago

Looks like the lint is blocked by https://github.com/CocoaPods/CocoaPods/issues/11558

wmcginty commented 1 year ago

@br-earl-gaspard @rmirabelli All the changes have been address, we should hopefully be good to go here now.