need a way to indicate async response
jaredly opened this issue · 21 comments
currently the requirement of returning _done doens't work with async responses. Would be nice to be able to return e.g. Express.asyncMarker or something which is just let asyncMarker: done_ = [%bs.raw "null"] or something
I have this in a separate project. I'll put it up for a PR later today. In the meantime, can you check out https://github.com/bassjacob/bassjacob.com/blob/master/src/server.re#L29 and let me know what you think?
@bassjacob I agree with you that the way forward would be to extend the middleware constructor function:
type asyncMiddlewareFn = Request.t => Response.t => Next.t => Js_promise.t done_;
external asyncFrom : asyncMiddlewareFn => Middleware.t = "%identity";@jaredly @bassjacob the solution would be more restrictive than what the express API can do since it can support pure callback, but I think it's good. Promise allows proper typing of the API.
especially for the moment, I think restricting to promises will be best (especially when lwt lands!!!). callbacks are awkward at best with reason ([@@bs.uncurry] or [@bs] everywhere) and the performance benefit really only matters at pretty massive scales. I'll open a PR with the code above in the next day or so.
Hi all. I'm getting stuck in this at the moment. Is there a work around for this problem? Or is built in promise support coming soon?
I would be in favour of a promiseFrom function here :)
I'll put together a builtin solution this weekend based on the link above
I'd also be interested in a solution to this. I think that asyncFrom solution is a good start, as I'm also returning Promises.
The ocaml-graphql has a really nice pattern where it allows the user to specify the effect type they want to use, and provides a default implementation for synchronous code. I think would be nicer than providing implementation specific functions.
https://github.com/andreas/ocaml-graphql-server/blob/master/graphql/src/graphql.ml
Does this need a done_ return type? Can't it just be unit?
The done_ return type is designed to ensure that all code paths respond to the request.
Nonetheless, changing it to unit, and nothing will prevent you from writing async handlers.
Adding promise support is fine, but I'm not in favor of a solution that requires you to use promises. I don't use promises internally in my Reason code, and express itself doesn't force the use of promises.
Those, that want a solution can clone from my git fork.
npm install --save git+https://github.com/PeteProgrammer/bs-express.git
I think sacrificing type safety here would be less than ideal. It's possible to keep type safety while supporting async handlers and not mandating one particular async type.
It all depends on how much safety we want to preserve. Using something like %identity% you I can cast any expression to an AsyncMiddleware done_ though I'm not sure how meaningful that would be. I'd rather support cb's, promises and synch methods inside this library, and then add any new ones people add.
The other possibility would be to make Middleware a Functor where done_ is contructed from a module's t like so
module Middleware = {
module type Input = {
/* elided */
};
module type Output = {
/* elided */
};
module Make(I: Input): (Output with type t = I.t) => {
type done = I.t;
let handler = I.handler;
};
};
but I'm not sure the added weight would be worth it.
@PeteProgrammer as @lpil changing it to unit exposes a potential bug that the type system helps with here: the case where you don't respond to the client. Since the only thing that can produce done_ types comes from inside this library, by restricting a middleware to returning a done_ type we can be sure the function definitely responds in some way.
I think this is a more common problem than wanting to use an async method that isn't a promise or a callback, but I'm interested in your use case. Is there a time where you wouldn't want to either respond directly, use a callback or use a promise?
@bassjacob - I do use callbacks, I just don't use promises. My async functions return a type async 'a = ('a => unit) => unit - where the 'a would most often be a result type.
And I do understand the problem that the done_ type helps solve, I think it's quite clever idea. But in my case, failure to call one of the send functions would be caught by a unit test.
@PeteProgrammer If this binding supported both callbacks and promises side by side would that satisfy the way you're using it? I'm loath to remove the done_ since I find it so useful to push into the type system rather than unit tests (one of the things I love about languages with type systems like OCaml).
I'm putting together a PR to support both promises and callback middleware types and will try submit it soon.
@bassjacob - It sounds like it would support my needs. Once you have a commit, I can try it out and give you feedback. (If you added the /lib/js folder to git, I would be able to add the npm package to my project directly from git)
As an input, here is how I currently use the the Api - slightly modified for easier readability (using my modified version that uses unit instead of done_, and I overshadowed the Request and Response functions to allow piping).
One thing worthy of notice here is that I have both a sync and an async path through the code. Sync in the case where I cannot parse the body as a JSON object.
Router.post(
auth,
~path="/register",
Express.Middleware.from(
(req, res, _) =>
switch (createUserInputFromRequest(req)) {
| None =>
res |> Response.status(400) |> Response.sendString("Invalid object")
| Some(input) =>
let asyncResult = input |> UserFeature.registerUser;
let callback = (result) => result |> ExpressApi.writeResult(res);
asyncResult(callback)
}
)
);
To make this type check just call YourIOType.return to wrap the sync value in the async context. :)