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

Change response type to response action #146

Closed
wants to merge 1 commit into from

Conversation

tmattio
Copy link
Collaborator

@tmattio tmattio commented Apr 11, 2020

Hi!

I wanted to use ocaml-graphql-server with opium and found that it required the use of Cotthp_lwt.Server.response_action to support the subscriptions.

To support this use case, this PR adds a field action to Rock.Response.t and change Opium.App.run_unix function to use Cohttp_lwt.Server.make_response_action instead of Cohttp_lwt.Server.make.

Unfortunately, as response_action is IO-dependent, I had to functorize most of Opium_kernel.. It complexities the use of Opium_kernel quite a lot, so if you have any idea of how to support response_action differently, I can update the PR with a different solution 🙂

For reference, I based my implementation of ocaml-graphql-server for cohttp on this one:
https://github.com/andreas/ocaml-graphql-server/blob/master/graphql-cohttp/src/graphql_cohttp.ml

And use it like this with opium handlers:

module Make (IO : Cohttp_lwt.IO) = struct
  module K = Opium_kernel.Make (IO)
  module Graphql_cohttp_lwt =
    Graphql_cohttp.Make (Graphql_lwt.Schema) (IO) (Cohttp_lwt.Body)

  let ( >|= ) = Lwt.( >|= )

  let graphiql (_req : K.Request.t) =
    match Assets.read "graphiql.html" with
    | Some body ->
      Lwt.return @@ K.Response.of_string_body ~code:`OK body
    | None ->
      Lwt.return
      @@ K.Response.of_string_body
           ~code:`Not_found
           "GraphiQL could not be found"

  let make_context _req = Graphql_context.default

  let api (req : K.Request.t) =
    let schema = Schema.schema in
    let callback = Graphql_cohttp_lwt.make_callback make_context schema in
    callback req req.request req.body >|= function
    | `Response (r, body) ->
      K.Response.of_response_body (r, body)
    | `Expert (r, _action) ->
      K.Response.of_response_body (r, `Empty)
end

@anuragsoni
Copy link
Collaborator

anuragsoni commented Apr 11, 2020

Hi @tmattio,

Thank you for this PR. I will try and take a look at this really soon. In the mean time, are you aware of any graphql implementation that works with httpaf? I'm not sure if you've seen #145 but the goal for opium is to switch to httpaf fairly soon. One reason i hadn't switched to response actions yet was because of the functorization. Ideally i'd like to avoid a functor based approach that'll then quickly be removed for httpaf. I'll try and think a little more about this problem too, and see what would fit nicely with the future goals of moving to httpaf.

Best,
Anurag

@tmattio
Copy link
Collaborator Author

tmattio commented Apr 11, 2020

I initially wanted to use your branch to use opium with httpaf, but apparently there is no good solution for web sockets with httpaf at the moment, which is a blocker to use the subscriptions in ocaml-graphql-server (for additional context: andreas/ocaml-graphql-server#190)

There's some work going on to make websocketaf work with httpaf (inhabitedtype/websocketaf@add-server...andreas:add-server2), but I don't know enough about it to know whether this will require opium_kernel to be functorize or not.

@anuragsoni
Copy link
Collaborator

Ah yes, websockets will indeed be necessary before the graphql library is supported.

but I don't know enough about it to know whether this will require opium_kernel to be functorize or not.

I don't think opium_kernel will need to be functorized on the httpaf branch. The types are defined in the httpaf library that is unix independant. The connection upgrades in httpaf will also happen in the server state machine in the core library (httpaf) which is unix independant. Given that there is an update to the PR i'd atleast like to wait a little while to see how that proceeds before making a decision on how to approach it in opium.

And while it isn't the nicest solution, if you are blocked on websocket/graphql at the moment you might be able to do something similar to https://gist.github.com/anuragsoni/909de157ab37a06d38ec8b2acd2bed81 to proceed with your explorations.

@anuragsoni
Copy link
Collaborator

I'm closing this for now since the active development will happen with httpaf instead of cohttp. We'll keep an eye on websocketaf for the websocket solution.

@anuragsoni anuragsoni closed this May 30, 2020
@tmattio tmattio deleted the response-action branch December 3, 2020 10:22
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