-
Notifications
You must be signed in to change notification settings - Fork 282
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
Transport layer abstraction #5
Comments
For reference, this also came up in the past at shurcooL/githubv4#2. However, the original author ended up no longer needing the feature, so I deferred thinking about it until a new concrete need would arise (such as this one). Currently, According to http://facebook.github.io/graphql/October2016/ and http://graphql.org/learn/serving-over-http/, the GraphQL specification doesn't require the use of HTTP, it just happens to be the most common protocol. But others are valid too. So, I think this is a valid request. We need to think about what a clean interface for this could look like. |
Just wondering, what GraphQL server are you using, and does it support non-HTTP transports? |
We're currently using neelance/graphql-go ontop of a WebSocket server. |
I'm also starting to pine for some pluggability. I'd like to start doing mocks. I don't know if this requires replacing HTTP per se, but it might be one of the most direct approaches. The project I'm working in already has a bunch of wrapper methods for our queries, so I could also just turn those into an interface, and implement a mock one which does some flipflops of json, etc. Having the pluggable part be in the graphql library's transport seems just as clean though, if not cleaner. Also, having the transport be pluggable might make it easier for me to write a snippet of code to update my own saved mock data by just letting the regular control flow run and saving it. Still thinking about this though. |
I did an experiment with this, and here's what I learned: It's hard to decide if the Transport interface should use structs... type Request struct {
Query string `json:"query"`
Variables map[string]interface{} `json:"variables,omitempty"`
}
type Response struct {
Data json.RawMessage
Errors errors
}
type Transport interface {
Do(context.Context, Marshaller, Request) (Response, error)
}
type Marshaller interface { /* ... */ } ... or if it should be the simpler, but less-typed binary pipe: type Transport interface {
Do(context.Context, []byte) ([]byte, error)
} My instinct is to prefer types over bytes, but implementing that seems to imply a need for a marshaller utility function that any transport implementation would call in order to be DRY and consistent. I also tried the plain bytes way, and it did involve less code... but I also almost immediately ended up writing code that wanted to presume json format of the bytes moments later, when I wrote a another transport implementation called So, I'm still on the fence about which of these makes more sense. Part 2: YES, using this for testing is really neat. I don't know how I lived before this. It's so convenient, and so useful! I made Then, simply adding some flags to my tests that switch between the So I'd like to turn this stuff into a PR. But I thought I'd write this up first -- and request some feedback about the interface, and structs versus bytes question, before cleaning my experiment up into a worthy PR. WDYT? |
Thanks a lot for experimenting with this and writing up those notes @eric-am, that is very helpful and valuable. I want to take some time to think the proposed API, read up a bit on GraphQL spec to see what they specify, and then I can share my opinions. We won't be able to merge a PR before making a decision here, so feel free to hold off on that. But you can send it earlier if you think seeing the code can provide additional insights. |
Would you mind elaborating on how it compares to the current testing method used in this package (i.e., |
Just for the sake of conversation the diff is here (with many things that are not meant for upstreaming, being too opinionated for that: dbmedialab-archive/go-graphql-client@master...pluggable-transport |
Note that for testing and logging, as in shurcooL/githubv4#36, it is not too hard to create a custom http.RoundTripper, and slide it into the http.Client that is fed to NewClient. If this transport layer wraps the default http.Transport by calling req.GetBody(), it is easy to do logging of the body. For testing, you can also insert a mock http.RoundTripper to do whatever you desire. That's a bit less ambitious than swapping out http.Client. But swapping the client may be what is needed for what graphql is doing. |
IO agnostic is quite important for me to adapt a library like this. Because I would always prefer to handling IO with my own strategies. Even though I am using HTTP for GraphQL, I would like to fine control the timeout, retries, exponential backoff, and logging. Therefore, I believe it's very valuable for this lib to be IO agnostic, as I consider this the best GraphQL client in Go atm. |
Facing the same issue, we ended up creating https://github.com/infiotinc/gqlgenc |
I also wanted to use this library in a transport-agnostic way, so I extracted the marshaling/encoding bits to another package: https://github.com/deref/graphql-go/tree/d269e7a529c689bc6fbd40814615844d9a1e47ef#encoding Thanks to @dmitshur for his excellent work on this original library! EDIT: Direct link to latest version of my fork: https://github.com/deref/graphql-go |
Currently looking at the Tibber Pulse api integration for https://evcc.io. Looks like #5 (comment) is the latest/ best suggestion for streaming API integration? |
For reference: my best experience regarding web sockets support (i.e. subscriptions) sofar has been https://github.com/hasura/go-graphql-client which is a fork. It would be nice to see something like that as part of this module. |
To be able to use other transmission protocols, such as WebSockets for example, shurcooL/graphql needs to be abstracted away from the actual transport layer.
This abstraction could also allow:
I suggest to define an interface with a default HTTP client implementation available out of the box and proper documentation for custom implementations.
Code-wise it could look somewhat like this:
func NewClient(transport *TransportInterface, scalars []reflect.Type) *Client
client := graphql.NewClient(graphql.NewHTTPTransport("/query", &http.Client{...}, nil)
The text was updated successfully, but these errors were encountered: