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

request / response serialization to message #4

Closed
tamagokun opened this issue Nov 18, 2014 · 11 comments
Closed

request / response serialization to message #4

tamagokun opened this issue Nov 18, 2014 · 11 comments

Comments

@tamagokun
Copy link
Contributor

I wanted to start a new issue thread for figuring out how we will serialize requests and responses into graft messages.

I see 3 problems we need to solve:

  1. We need to be able to access important data in req. (url, params, etc)
  2. Set status code and headers on the response.
  3. Provide access to the req/res streams so you could pipe an http request/response somewhere else.

Below is an ongoing draft of what these message blocks would look like:

// request
{
    httpVersion:
    method:
    url:
    host:
    hostname:
    path:
    protocol:
    port:
    query:
    params:
    body: req
    res: graft.ReadChannel()
}


// response
{
    statusCode:
    headers:
    body: // string, buffer, stream
}

I think 1 and 2 are already solved, we just need to decide on the message properties. For 3, I don't know how to best handle this. We would have a request stream, response stream, but also a return channel? Is there a way to merge these somehow?

@GraftJS/graft-http-committers

@mcollina
Copy link

I think you should have a ret channel on the request, not the httpResponse. On that ret channel, graft-http will expect what you identified as "response" in your example, and body can be:

  1. a string
  2. a buffer
  3. (default) a stream, in that case it's piped to the httpResponse

Does it make any sense?

@mcollina
Copy link

I think we should have the HTTP body parsed for JSON, ecc.. otherwise doing crazy things will be hardish. However we should be able to disable that by turning a flag, or something similar.

@tamagokun
Copy link
Contributor Author

Makes sense!

We can take a look at the content-type of the request and parse the body appropriately. The bridge function can accept a hash of options to disable this, which would just pass the request stream through.

@tamagokun
Copy link
Contributor Author

I'm also assuming that since graft-http will send the message right when the request comes in, the body will still be a stream, even if it is getting parsed as json, form data, etc. You would have to listen for data or end to get the data from the request. Maybe there is an option to wait for the request to be finished before sending the message.

@mcollina
Copy link

@tamagokun I'm not sure on that. Otherwise a validation step might be tricky, and that's going to be parsed anyway for doing anything serious.

@tamagokun
Copy link
Contributor Author

Or if we want the body parsed, it will wait until the request is finished before sending the message?

// parse: true
// graft sends message once request stream emits end. contents of body is string or object
graftHttp.use(service(), {parse: true});

@mcollina
Copy link

I definitely think so.

@AdrianRossouw
Copy link
Contributor

how much of this is going to be re-inventing connect/express though ?

perhaps we should focus on adapting connect middleware to be used?

@tamagokun
Copy link
Contributor Author

Exactly. middleware micro-service. Something that could wrap up a connect middleware into a micro service that is consumable by graft. It would be great to create something that could achieve full interoperability with any http based middleware, but I haven't done any tests yet.

All of the routing and parsing we are discussing could be handled by piping the request through to other graft-compatible middleware rather than using options.

@tamagokun
Copy link
Contributor Author

Converting requests to/from messages: https://github.com/GraftJS/graft-http/blob/draft/lib/request.js

As I mentioned in #1, I need a better way to handle serializing the http request. Middleware and other things love to tack on properties to that and share them with other middleware, so I was imagining a black list of properties that we would throw away, and just keep the rest (plus a few convenience ones that aren't normally a part of http.IncomingMessage)

@tamagokun
Copy link
Contributor Author

This should be all set now. If message keys need to change at all, open a new issue.

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