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

[WIP] Networktransport idea #6

Closed

Conversation

lightsprint09
Copy link

This pull request contains two significant changes.

1. Changes the NetworkTransport to a real NetworkTransport.

NetworkTransport should not do parsing or handling domain specific errors. It only hits the network and returns with you requested data. It should not know anything about GraphQL at all.

2. Users of the API should be able build their own service.

All they need is a function to create an URLRequest from an operation with a given baseURL and a function which parses a JSON root object (or NSData) back to a GraphQLResult.

Those 2 described ideas can be helpful when the user of your API wants to use his own infrastructure.

@martijnwalraven
Copy link
Contributor

Thanks for bringing this up! I like the idea of making NetworkTransport more flexible, but before getting into specific designs, maybe we can look at the use cases we want to support first. Do you have any specific requirements in mind when you mention using the client with your own service/infrastructure?

The reason for making NetworkTransport responsible for the parsing and error handling is that there are quite a few possible extension points when going from a GraphQL request to response, and because we don't know what those are yet, I lumped all of them together in one high level protocol.

I deliberately defined NetworkTransport at the level of GraphQL operations because I wanted it to be able to perform GraphQL specific processing, and there can be considerable flexibility there. No transportation mechanism for GraphQL is defined in the spec, although fortunately there is a consensus around simple usage based on what express-graphql does. But you can imagine different mechanisms for query batching or authentication for example. I also wanted to leave open the possibility of supporting alternative GraphQL serialization/response formats (there has been talk of adding partial/delta responses for instance).

I'd also prefer NetworkTransport to not be limited to HTTP. To support subscriptions, you would use WebSockets for instance, and you may want to send regular requests over that same connection instead of opening a separate HTTP connection for that.

I haven't thought about this in detail, so these are just some first thoughts. Maybe we can identify some useful extension points and take it from there.

@lightsprint09
Copy link
Author

Ok, I understand that. But a NetworkTransport should only do networking. I would recommend to put all your mentioned GraphQL logic into the client. After this reloacting the code I would try to separate the client in different pieces e.g parsing, loading, etc. All these pieces could be interchangeable.

When I find time I try to model some interfaces

@martijnwalraven
Copy link
Contributor

I agree it makes sense to move some behavior around and split extension points up in different protocols, but I don't agree NetworkTransport has to be independent of GraphQL.

One way of looking at this is that NetworkTransport represents what a GraphQL client requires from a network transport, rather than some general purpose networking layer. This is similar to the way Apollo Client and Relay approach this. You can plug in in any transport, as long as it allows you to send operations, and it can do transport-specific processing if needed (like batching, or even using an alternative serialization mechanism).

@MrAlek
Copy link
Contributor

MrAlek commented Feb 1, 2017

We're currently using Apollo iOS with our own network stack & caching layer because we have other HTTP services in the app and want to centralize the networking aspect. I agree with @lightsprint09 on the architectural changes clearly separating the GraphQL layer from the network layer.

As a framework consumer, I only want to use Apollo to serialize and deserialize GraphQL requests & responses to & from raw Data. Dealing with networking properly is a tough problem in iOS and can be solved in many different ways which is why it's important to let each app developer be able to decide on their own stack. With multiple, heterogenous stacks it gets hard to create good abstractions on the application level so you can switch pieces out independently, say if you regret the decision of using GraphQL in the first place and fall back to REST services.

@martijnwalraven
Copy link
Contributor

@MrAlek @lightsprint09: I'm happy to continue this conversation and see how we can make the framework more flexible.

Most importantly, I'd like to better understand where the current design falls short, especially since we're starting to think about adding more network-level features like authentication, query batching and persisted queries. A WebSocket-based transport for subscriptions (and potentially all operations) is also on the roadmap.

Right now, NetworkTransport defines a pretty minimal interface from the client to the networking layer:

public protocol NetworkTransport {
  func send<Operation: GraphQLOperation>(operation: Operation, completionHandler: @escaping (GraphQLResponse<Operation>?, Error?) -> Void) -> Cancellable
}

HTTPNetworkTransport then offers a standard implementation that uses URLSession. If you want to hook into another networking layer, all you need to do is provide your own implementation of NetworkTransport.

Most of the code in HTTPNetworkTransport is specific to URLSession, and the rest is glue code that uses existing helpers for things like serializing/deserializing variables and results. So it feels this is already pretty reusable from your own network transport.

What do you feel is missing from the current design, and how do you suggest we should improve it?

@MrAlek
Copy link
Contributor

MrAlek commented Feb 2, 2017

@martijnwalraven Me and @fwal (#46) are working on the same project and for us, even the NetworkTransport protocol is too strict because it is controlled by ApolloClient which dictates when the request should hit the network, enforcing one particular scheme for handling asynchronicity.

In our implementation, we don't use ApolloClient at all but just serialize requests from the generated GraphQLOperation subclasses and then deserialize with GraphQLResponse. This way, we can store requests, queue them up using Operation subclasses and let the application layer handle all HTTP requests uniformly. We can then do things like inject authentication tokens when the request is actually running rather than when it's created.

The things you have on the roadmap for the network side of things in the client are great but there should still be an easy way to opt-out and just use the core serialization/deserialization of GraphQL operations for easier integration in existing apps.

Example of our usage:

static func request<O: GraphQLOperation>(_ operation: O, authenticationLevel: APIAuthenticationLevel, url: URL) throws -> HTTPRequest<O.Data> {
    let map: GraphQLMap = ["query": type(of: operation).queryDocument, "variables": operation.variables]
    let data = try JSONSerialization.data(withJSONObject: map.jsonValue, options: [])
    let body = HTTPBody(data: data, contentType: .json)

    return HTTPRequest<O.Data>(
        url: url,
        method: .post,
        body: body,
        authenticationLevel: authenticationLevel,
        responseTransformer: { response in
            // Error handling & response transformation using GraphQLResponse
        }
    }
}

@martijnwalraven
Copy link
Contributor

martijnwalraven commented Feb 2, 2017

@MrAlek @fwal: I think it would be better in the longer term if we could make the interfaces flexible enough to allow for your use cases. Opting-out of using ApolloClient may be the right call for you now, but you're missing out on some awesome features (like the recently added normalized caching and query watching) 😊.

So I'm definitely open to suggestions if you feel there is anything missing in the API that blocks you from only using the library for serialization/deserialization, but I'd also like to better understand what restrictions you see that keep you from using it through the public API.

the NetworkTransport protocol is too strict because it is controlled by ApolloClient which dictates when the request should hit the network, enforcing one particular scheme for handling asynchronicity.

I'm not sure what you mean by this. Since send(operation:) is asynchronous, it is up to the network transport to call the completion handler whenever there is a response available. And it is free to initiate the network request whenever it wants.

This way, we can store requests, queue them up using Operation subclasses and let the application layer handle all HTTP requests uniformly. We can then do things like inject authentication tokens when the request is actually running rather than when it's created.

What keeps you from doing this within the send(operation:) method? Couldn't you just create and queue up an Operation instance from your network transport?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants