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

Suggestions #1

Open
andreas opened this issue Jun 7, 2018 · 5 comments
Open

Suggestions #1

andreas opened this issue Jun 7, 2018 · 5 comments

Comments

@andreas
Copy link

andreas commented Jun 7, 2018

Thanks for sharing your code, @anmonteiro! 🙏It's great to have some more examples out there.

Two quick suggestions/questions:

  • Could some of the usages of io_field be replaced with field to avoid Lwt_result.return, e.g. this one?
  • Could you use use HTTP server that ships with graphql-lwt, or does it lack some feature compared to the one you implemented? At a glance they seem quite similar.
@ozanmakes
Copy link

@andreas

Could you use use HTTP server that ships with graphql-lwt, or does it lack some feature compared to the one you implemented? At a glance they seem quite similar.

I'm guessing it's because of the static file serving logic. I had to do something similar to customize the included GraphiQL page.

Is there a way to include the module and override the assets/route handlers without vendoring graphql_lwt?

@anmonteiro
Copy link
Owner

@osener that's indeed the reason; @andreas and I talked on Discord about it and I never responded in this issue.

Is there a way to include the module and override the assets/route handlers without vendoring graphql_lwt?

I think that's what this issue is for, correct me if I'm wrong: andreas/ocaml-graphql-server#93

@andreas
Copy link
Author

andreas commented Jul 10, 2018

If I understand you correctly, what you're looking for is a way to compose the GraphQL HTTP handler with other routes. To that effect, Graphql_lwt.Server could expose a function with the following type

'ctx Graphql_lwt.Schema.schema ->
Cohttp.Request.t ->
Body.t ->
'ctx ->
(Cohttp.Response.t * Body.t) Lwt.t

Is it something like that you're looking for?

I don't consider andreas/ocaml-graphql-server#93 to be about such a function, that's simply moving the HTTP server to it's own OPAM package.

@anmonteiro
Copy link
Owner

That sounds like a good idea but I'd still put it in the separate package that andreas/ocaml-graphql-server#93 is about.

If I wanna use ocaml-graphql-server with Httpaf, for example, I wouldn't want there to be Cohttp dependencies in the dependency I'm pulling.

@andreas
Copy link
Author

andreas commented Jul 10, 2018

@anmonteiro that makes sense 👍

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

No branches or pull requests

3 participants