-
Notifications
You must be signed in to change notification settings - Fork 88
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
Switch to Alamofire. Fixes #29 [skip ci] #30
Conversation
34b9799
to
5012b2c
Compare
It passes tests! @1ec5 care to review? Main point of "huh" is the working around dispatch_sync https://github.com/mapbox/MapboxDirections.swift/pull/30/files#diff-e76be193f7c380a503afe0e47c1cd829L132 - keeping these calls in would hang the main thread, and think that's because Alamofire is already on the main thread so maybe like it's waiting for itself? Really I have no idea. But I removed the calls willy-nilly and am not sure if it works, only sure that it makes tests pass. |
@@ -36,10 +37,7 @@ public class MBDirections: NSObject { | |||
|
|||
private let request: MBDirectionsRequest | |||
private let configuration: MBDirectionsConfiguration | |||
private var task: NSURLSessionDataTask? | |||
public var calculating: Bool { |
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 should keep this property for compatibility with MapKit. It's how client code knows that the object is already waiting for a request. Perhaps not the most elegant approach, but we should be deliberate about the places we diverge from MapKit.
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.
Per voice, given the async'ness of requests this bit of compatibility with MapKit isn't worthwhile: our directions SDK doesn't have any good reason will permit concurrent requests and any kind of serialization would happen on the app level.
From a quick glance at the Alamofire codebase, I think you're right that it already handles dispatching to the main queue. RequestKit did not, so we had to dispatch to the main queue ourselves; otherwise, client code would end up executing on the URL session delegate queue. |
👍 got rid of the big switch statement I'm going to do the user-agent header addition in this branch, but feel like a v4/v5 strategy refactor is better left for another day, since it's pretty involved and also would diverge from the original API even more. |
Testing for the User-Agent header is tricky because Nocilla doesn't support expectations on requests: luisobo/Nocilla#4 |
I switched from the |
I ended up switching back to OHHTTPStubs/Swift in #47, along with rewriting the library in a way that addresses the weirdness above but also makes it more difficult to integrate Alamofire into the library. |
Closing per #29 (comment). |
No description provided.