Add HTTPNetworkTransport.init to take URLSession instance to allow … #265
Conversation
…caller to handle exceptional cases by way of URLSessionDelegate
|
@chrisco314: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
|
I don't think we can allow passing in an existing session, because we want |
|
Hey @chrislconover - Thank you for doing this, and sorry for the long radio silence on our end! Can you please expand on the use cases here? Certificate setup seems to clearly be one, but I want to see if there's other stuff that isn't covered by the changes in #602. |
|
@chrislconover Any thoughts on this? If I don't hear back within another week I'll close this PR out in favor of the changes that have shipped with 0.11.0. |
|
I don't have the calling code in front of me, but effectively you are breaking the existing interface, by blocking access to session configuration. I think I was having to deal with authentication challenge issue that are handled through the session and session configuration. It's been a while now, but in my collection, you broke the interface by wrapping with with an opaque layer, preventing one from accessing the existing URL session functionality. It is generally unsafe and unwise to assume that nobody will need the existing flexibility and then wrap it with an opaque layer - this is generally considered poor API design. |
|
@chrislconover OK, it's helpful to understand the use case. We're still working through a lot of the decisions that were made when the library was first created (and wasn't used as widely for things which weren't Apollo products). There was a time when we were trying to avoid passing in the session since we wanted to have the option of being the session's delegate ourselves, but I need to talk through with some colleagues the implications of removing that ability before we allow that. |
allows HTTPNetworkTransport to take URLSession instance This brings apollographql#265 into our fork. We need this capability so we can handle the session's `URLAuthenticationChallenge`.
|
@chrislconover OK after some discussion we are changing direction and letting this in. In practice, we've never been using the delegate properties of the session, and not allowing the user to pass in their own session leads to all kinds of problems, including making background sessions really hard to use, making mocking a total pain in the butt, and making it impossible to either certificate pin or use a self-signed certificate. Can you please fix the merge conflicts and then we can (finally) get this shipped? |
|
@chrislconover If I don't hear from you by tomorrow, I'll close this out and start a separate PR with this functionality. Thanks! |
|
Please do whatever is easiest for you. I don't need or want credit or copyright, and hereby revoke any claims in perpetuity to any and all of my changes to the Apollo codebase. Go nuts! -c |
Add HTTPNetworkTransport.init to take URLSession instance to allow caller to handle exceptional cases by way of URLSessionDelegate. The problem was that HTTPNetworkTransport was hiding and subsequently blocking access to existing functionality on URLSession.
This change is to add an additional initializing that takes an URLSession instance from the caller, allowing said caller to first register a delegate handler. By contrast, Alamofire handles this in arguably a more elegant method, by exposing optional closure methods to handle each of the three URLSessionDelegate methods.
The approach taken here is simpler to implement, but is less elegant for calling code. It is more conventional though and will suffice until any higher level solution decision is warranted.