Add httpResponse property to GraphQLResponse object in order to get access to its headers

[ghost]

No description provided.

[apollo-cla]

@TomaszWlodarczyk: 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/

[martijnwalraven]

Thanks for submitting a PR!

I don't think we can expose the HTTP response on the GraphQLResponse directly, because that ties it to a specific transport. There is ongoing work to add support for the WebSocket transport for example, and that wouldn't work with this.

Maybe we can add a more generic context to GraphQLResponse that transports can add data to? What is your use case exactly? What headers would you like access to?

[ghost]

The thing is that we pass over from backend to the app a unique response ID as a custom HTTP header in order to trace it later in case of any issues. That's why I need to have an access to that. Is there any other way to do so?

But you do that in case of response failure anyway....

Anyway adding all headers from HTTPResponse to GraphQLResponse would solve may case

[martijnwalraven]

@knutaa @MrAlek are working on the WebSocket transport and changes to the networking layer, so they may have more specific ideas. A simple solution might be to add a context map to GraphQLResponse, and have HTTPNetworkTransport set headerFieldson it.

[knutaa]

Regarding generalization of network transport, one way of handling this could be to have the response as an enum (of transports) with associated data. This new enum could then be associated with the server value of the GraphQLResult.Source to be available to the client.

[ghost]

Is there any chance that you address that in near future?

[MrAlek]

I also agree that HTTP headers should be specific to the HTTPNetworkTransport because it's not really part of the GraphQL operation itself (nor the GraphQLResponse).

Another alternative would to be to have a delegate protocol on HTTPNetworkTransport with a callback like
didReceiveResponse(_ response: HTTPURLResponse, for operation: GraphQLOperation) where you would extract headers if you need them. Would that work in your case?

[ghost]

@MrAlek yeah that would work!

[knutaa]

To avoid having the client coordinate between two async callbacks (headers and the normal response) and to support multiple transports, the following could also be considered.

The main idea is to have the enum Source take associated data (new TransportMessage) for the server value and the TransportMessage is itself an enum.

See attachment.
ios - apollo - transportmessage.pdf

[ghost]

Any ETA for introducing that?

[knutaa]

I have added the proposal based on GraphQLResult.Source to https://github.com/knutaa/apollo-ios. Should be possible to test based on that branch.

But believe some agreement is needed on design before this is progressed

[martijnwalraven]

Hmmm, I don't really like the idea of coupling the Source enum to particular transports. It also puts the burden on callers to interpret these for every result. What about using a delegate method to extract transport-specific information? Something like:

type Context = [String: Any]

func networkTransport(_ networkTransport: HTTPNetworkTransport, contextFor response: HTTPURLResponse): Context

And then we could pass through the returned context to GraphQLResponse and GraphQLResult.

[sfla]

I've just recently started using Apollo, so I don't have any super smart suggestions. At first glance, I'd think the most natural place to put a HTTPResponse is in a subclass of GraphQLResult named GraphQLHTTPResult which simply includes the response. From my point of view, it would go along nicely with the potential error-object GraphQLHTTPResponseError that can be returned (which does have the HTTPURLResponse). Each object that can be accessed in the fetch's resultHandler (when using a HTTPNetworkTransport) should expose the actual response in top level, shouldn't it?..
Then again, I probably have no idea what I'm talking about.

Anyway, in my case I deal with data for real-time public transportation, converting departure times to "departs in X minutes", and it is very important to be able to access the server's Date-header to prevent misinformation when users have set their device's clock a few minutes back or forth.

[knutaa]

I have created a new PR #266 based on the suggested use of context. But without use of the suggested delegate. Not sure if that is adding required flexibility as this time.

[designatednerd]

Hey deleted account friend - Thank you for doing this, and sorry for the long radio silence on our end!

Unfortunately since so much has changed since this PR was opened, I'm going to close this in favor of the work happening in #602 to implement this feature. Please feel free to chime in there if you have thoughts or questions, and open issues on anything you feel we may be leaving out.