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

Generalize graphql-cohttp to support other backends #190

Closed
wants to merge 2 commits into from

Conversation

tmattio
Copy link

@tmattio tmattio commented Apr 9, 2020

Hi!

I wanted to use ocaml-graphql-server with opium (the Httpaf version), so I generalized the implementation of graphql-cohttp to support other request and response types.

The goal is to make it easy to create a GraphQL server with any framework (httpaf, opium, re-web, etc.)

The current implementation lacks support for subscriptions, but I wanted to know if you were interested in the contribution before I continue: I'll finish it if you are 🙂

A few changes I made also:

  • The callback will not check the route (graphql-cohttp matches on "/graphql"). I think it makes sense to let the user handle the routing.
  • The server will not serve GraphiQL.
  • The returned value from the callback is a HttpResponse.t, instead of
    [ `Expert of Cohttp.Response.t * (Io.ic -> Io.oc -> unit Io.t)
    | `Response of Cohttp.Response.t * Body.t ] Io.t

in graphql-cohttp. So the user will have to wrap to callback with a bit of custom code when using cohttp.

@andreas
Copy link
Owner

andreas commented Apr 9, 2020

Hi @tmattio!

Thanks for this 😄 I always intended to support httpaf, but the lack of websocket support has been a blocker. This is why I worked on getting websocketaf into a workable state about a year ago (PR here). I haven't checked recently, but I believe there's still no viable websocket solution for httpaf.

About the approach. I would prefer keeping graphql free from transport concerns, and delegate that to other packages. This would likely mean having three packages:

  • graphql-http: Package that's specific to using HTTP as the transport mechanism for GraphQL without being tied to a particular HTTP library.
  • graphql-cohttp: Package for integrating graphql and cohttp leveraging graphql-http.
  • graphql-httpaf: Package for integrating graphql and httpaf leveraging graphql-http.

If graphql-http ends up being too slim, it might not be worth it compared to duplicating a bit of code. If you have other ideas on how to avoid a proliferation of packages, I'm all ears 😅

@tmattio
Copy link
Author

tmattio commented Apr 11, 2020

Oh! I saw websocketaf before I started on this and wrongly assumed it was working 😕
Very happy to see that you revisited your PR on websocketaf, thanks a lot for all your work!

Having graphql independent from the transport layer makes complete sense, and the introduced Graphql_server could lie in a separate package.
I don't think a graphql-http package would end up being too slim to exist. I used the provided Graphql_server functor to implement a GraphQL server for opium and found it very useful!
The package could also let some of the responsibilities taken by graphql-cohttp to the user. For instance, if the user wants to do its own routing, they could use graphql-http instead of the ready-to-use server provided in graphql-cohttp.

About the proliferation of packages, maybe it makes sense to provide packages for popular servers (cohttp and httpaf) and let users implement their own graphql server if they are using another library/framework.

I could still use ocaml-graphql-server for my use case since I'm not using subscriptions (for now), but it probably does not make sense to go forward with this until ocaml-graphql-server can be used with httpaf.
I'll be happy to give a shot at implementing a graphql-http package when/if you want though 😄

@tmattio
Copy link
Author

tmattio commented Oct 8, 2022

I'm cleaning up my open PRs and this doesn't seem relevant anymore, so closing now 🙂

@tmattio tmattio closed this Oct 8, 2022
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.

2 participants