reason-native-web / morph Goto Github PK

Hi! Your repo is referring to the undocumented AZP_CACHING_TAR env var. We're rolling out TARing as default with agent 160. If this is what you want, you can just remove this env var. If you want to specifically not use TARing, then set AZP_CACHING_CONTENT_FORMAT to Files. We'll be documenting this new environment variable.

CC @fadnavistanmay

The resolutions make it quite painful to integrate morph in a project. Is the antonio's httpaf fork required? Can it be replaced with anything from opam/esy (h2?)?

I had some trouble writing handlers beyond the provided examples. This could be because of the rapid changes seen in the repository.

The following never worked

req.request.target
|> Uri.of_string
|> Uri.query

Unbound 'target'

I'm on 0.6.1

It would be great to provide ~graphiql: bool option to Morph_graphql_server.make or provide something like Morph.Response.text(Library.GraphiQL.html, Morph.Response.empty) available from Morph_graphql_server module

/ cc @cem2ran

Problem

Currently Morph.Response.t is a result(Piaf.Response.t, failure). Failure is defined as

type failure = [
  Piaf.Error.t
  | `User(string)
  | `Server(string)
  | `Auth(string)
];

It's not entirely clear to me what the added value is of this extra result wrapping. Mostly because I'm unsure when I would receive a failure from Morph vs a response (a search showed my it's only really in the MSession middleware). As a simple test an incorrect query to a GraphQL server seems to return an internal server error (but not a Morph failure).

To illustrate the difficulty caused by the wrapping. If I want to output the status code of a response I need the following snippet.

let response_to_status : Morph.Response.t => int = (morph_res) => {
  (switch(morph_res) {
    | Ok(res) => res
    | Error(f) => f |> Morph.Response.response_of_failure
  }).status |> Piaf.Status.to_code
};

Suggestion

Remove the indirection around the response and make it a standard to use error responses for unrecoverable errors.

It's probably better to write tooling to help determine if a response is successful or not (if that's needed). Additionally, by always dealing in Piaf responses (or a thin, non-result wrapper with additions around that) we open up the possibility for Middleware to "save" a request using error correction.

The term middleware is already plural so there's no need for the s

This would be a breaking change.

The current pretty printing of routes are very simple so we should be able to do it without Pastel.

We need a way to support websockets.

For httpaf we can use this to upgrade the connection: https://github.com/anmonteiro/httpaf/blob/fork/lib/httpaf.mli#L640.
For h2 we can just take the TCP connection and do the same thing, if we want to support it at all.

The biggest problem is how to build the API.
My current thought is that we should add a function to the request to get hold of a socket (basically a way to send and receive data, maybe via Lwt_stream.t). And then to close the connection we return the response, but that might feel weird since we've actually already responded.
Another solution would be to have a Socket body that does the same thing but then the term body is overloaded.

Are these example still in the repo? I can't seem to run them. Thank you.

~/Github/morph master
❯ esy build
info esy build 0.6.6 (using package.json)

~/Github/morph master
❯ (esy examples-path)/hello_world.exe
fish: Command substitutions not allowed
~/Github/morph master
❯ esy examples-path/hello_world.exe

~/Github/morph master
❯ esy bin/hello_world.exe

~/Github/morph master
❯ esy examples
    ocamlopt docs/markdown/simple_switch_based_router.exe
ld: warning: directory not found for option '-L/opt/local/lib'
    ocamlopt docs/markdown/writing_middleware.exe
ld: warning: directory not found for option '-L/opt/local/lib'
    ocamlopt docs/markdown/with_routes_package.exe
ld: warning: directory not found for option '-L/opt/local/lib'
    ocamlopt docs/markdown/graphql_server.exe
ld: warning: directory not found for option '-L/opt/local/lib'
    ocamlopt docs/markdown/getting_started.exe
ld: warning: directory not found for option '-L/opt/local/lib'

~/Github/morph master
❯ esy docs/markdown/simple_switch_based_router.exe

~/Github/morph master

Needing http2 is probably not something we have to support. We should just delete this code.

Another solution is to refactor it so that you pass in a server implementation from the outside, but this might be a larger issue.

If anyone needs this support please comment here.

Found in README.md

It would be amazing to have some basic logging middleware available out of the box. Maybe not something fully featured like https://github.com/expressjs/morgan but basic logger middleware from https://github.com/reason-native-web/morph-hello-world-pesy-template/blob/master/library/Middleware.re would be a good start.

We currently have this code:

  let listen_address = Unix.(ADDR_INET(inet_addr_loopback, port));

This will result in that we only listen to localhost. We actually want to make it possible to choose this option when starting the server which we currently can't do.

Basically add a option to the interface that makes it only listen to localhost.

Documentation:
https://caml.inria.fr/pub/docs/manual-ocaml/libref/Unix.html#TYPEinet_addr

https://reason-native-web.github.io/docs/morph/writing-middlewares makes a reference to Morph.Method.to_string but this doesn't actually seem to be in the library.

With some digging I found that the method information from Piaf is actually hidden under request.request.meth so maybe we want to somehow map this to request.method directly (changing Morph.Request.t).

I came up with the following which appears to work.

let method_to_string : Piaf.Method.t => string = fun
  | `GET => "GET"
  | `POST => "POST"
  | `PUT => "PUT"
  | `DELETE => "DELETE"
  | `HEAD => "HEAD"
  | `OPTIONS => "OPTIONS"
  | `TRACE => "TRACE"
  | `CONNECT => "CONNECT"
  | `Other(s) => {j|UNKNOWN($s)|j}
  ;

/**
 * Creates a new logger middleware.
 *
 * The output of this function can be passed to the middlewares
 * argument of `Morph.start`.
 */
let make = () => {
  (service) => (request: Morph.Request.t) => {
    open Lwt.Infix;
    let start_request = Mtime_clock.elapsed();
    service(request)
    >|= (
      response => {
        let end_request = Mtime_clock.elapsed();
        Logs.info(m =>
          m(
            "http: %s request to %s finished in %fms",
            request.request.meth |> method_to_string,
            request.request.target,
            Mtime.Span.abs_diff(start_request, end_request)
            |> Mtime.Span.to_ms,
          )
        );
        response;
      }
    );
  };
}

We need to refactor it so that we have a http client and a https client and then figure out which to use and call it.

I wanted to try setting up a new project with pesy and this new framework and followed the instructions:

pesy --template=https://github.com/reason-native-web/morph-hello-world-pesy-template

I got the following errors:

➜  Reason pesy --template=https://github.com/reason-native-web/morph-hello-world-pesy-template
'esy build' failed. Could not build project.
Logs can be found in pesy.stdout.log and pesy.stderr.log

➜  Reason cat  pesy.stdout.log
dune: unknown option `-r'.
Usage: dune build [OPTION]... [TARGET]...
Try `dune build --help' or `dune --help' for more information.

➜  Reason cat pesy.stderr.log
info esy build 0.6.0-df516a (using package.json)
error: command failed: 'dune' 'build' '-p' '-reason' (exited with 1)
esy-build-package: exiting with errors above...
error: build failed with exit code: 1

We want to support Windows at least the client but preferably everything.