Skip to content
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

A per request/response context object passed among delegate callbacks #1132

Closed
xma000 opened this issue Apr 8, 2020 · 6 comments
Closed
Labels
enhancement Issues outlining new things we want to do or things that will make our lives as devs easier networking-stack
Milestone

Comments

@xma000
Copy link

xma000 commented Apr 8, 2020

We have a use case where we want to process HTTP headers and/or status code along with the GraphQL errors for a response. The callback for each of HTTPNetworkTransportTaskCompletedDelegate and HTTPNetworkTransportGraphQLErrorDelegate only contains information belonging to that layer, which is the right design. However, that means I won't receive information from different layers in one callback.

I propose that we add a per request context object to HTTPNetworkTransport. This mutable context object can hold attribute objects identified by string keys. This context object will be passed in every delegate callback for the request/response. This is a common technique to allow processors in multiple layers to coordinate. I think this is a great enhancement to the client library and is not difficult to implement.

With this support, we can set the header value to the context when we receive the callback through the interface of HTTPNetworkTransportTaskCompletedDelegate, then retrieve this value from the context when we process the callback for HTTPNetworkTransportGraphQLErrorDelegate.

This issue is a follow up to #1116.

@designatednerd
Copy link
Contributor

I'm gonna think this through - I think this might be more work than is worth it if the headers are the one thing everyone actually needs access to.

I know the Context is more flexible but it's wayyyy less type-safe.

@designatednerd designatednerd added the enhancement Issues outlining new things we want to do or things that will make our lives as devs easier label Apr 11, 2020
@sneakyness
Copy link

It would be incredible of this context also included away to get at the NSProgress of the underlying network request. Have been thinking about how best to expose this myself, was considering a similar pattern but I've got no idea what happens/how to handle when the response is cached or optimistically updated.

@designatednerd
Copy link
Contributor

For transparency's sake: I'm looking at a wholesale revamp of our HTTP request system to get something a bit more in line with the OKHTTP-inspired interceptors in our Android SDK and/or ApolloLink in our web SDK.

Basically, something where you can create a series of objects which do work for you and add your own arbitrary ones in as you want - this will remove the need for the gazillion delegate callbacks and give information at each stage of the process about the request and its associated response.

Still at the noodling phase but realistically this needs to be done anyway because the delegate situation is getting pretty out of control, and we also need to make some significant changes to allow use of background URL sessions.

I'm hoping to post a call for feedback in the next week or two - obviously trying to do two things at the same time with the codegen thing is a pain, but from the issues being raised here it does seem like this could have a big impact (and ultimately make it easier to use old vs new codegen, I hope).

@designatednerd
Copy link
Contributor

Please check out the networking update RFC #1340 @xma000 @sneakyness on this - I'm going to close this out when the network rewrite ships but I believe that should solve the issues you're encountering here.

@designatednerd
Copy link
Contributor

Hi, our new networking stack will be shipping as a beta soon and I believe it should solve the issues you've encountered here. Please see #1341 for the current implementation.

@designatednerd designatednerd added this to the Networking Beta milestone Sep 9, 2020
@designatednerd
Copy link
Contributor

The networking stack is now available as a beta at 0.33.0-beta1 (available via the betas/networking-stack branch and through PR #1386). Going to close this out, please open a new issue if your problems aren't addressed by the new networking stacks.

@designatednerd designatednerd modified the milestones: Networking Beta, Next Release Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues outlining new things we want to do or things that will make our lives as devs easier networking-stack
Projects
None yet
Development

No branches or pull requests

3 participants