shipharbor / merry

:ocean::ocean::sailboat::ocean::ocean: - cute streaming API framework
MIT License
312 stars 21 forks source link

add deserializers #20

Closed yoshuawuyts closed 7 years ago

yoshuawuyts commented 7 years ago

would be cool if we could add a JSON deserializer for fun and profit; seems like a common enough thing


todos

yoshuawuyts commented 7 years ago

for json we should use https://github.com/mcollina/fast-json-parse

yoshuawuyts commented 7 years ago

probably couple it with concat-stream and put it behind merry.parse.json or smth

yoshuawuyts commented 7 years ago

for form bodies we should use https://github.com/yoshuawuyts/multipart-read-stream/issues/6 and put it behind merry.parse.multipart I think

yoshuawuyts commented 7 years ago

API draft:

var merry = require('merry')
var pump = require('pump')

var app = merry()
app.router([
  ['/json', {
    'put': jsonRoute
  }],
  ['/multipart', {
    'put': multipartRoute
  }]
])
app.listen(8080)

function jsonRoute (req, res, ctx, done) {
  var jsonstream = merry.parse.json(jsonHandler)

  pump(req, jsonstream, function (err) {
    if (err) return done(merry.error('error parsing JSON'))
  })

  function jsonHandler (json) {
    ctx.json = json
    done(null, 'done parsing json')
  }
}

function multipartRoute (req, res, ctx, done) {
  var type = req.headers['content-type']
  var filestream = merry.parse.multipart(type, fileHandler)

  pump(req, filestream, function (err) {
    if (err) return done(merry.error('oh no!'))
    done(null, 'done parsing files')
  })

  function fileHandler (fieldname, file, filename) {
    ctx.files = ctx.files || {}
    var buf = new Buffer()
    ctx.files[filename] = buf
    file.on('data', buf.push)
  }
}

cc/ @lrlna @davidmarkclements - thoughts on the API?

davidmarkclements commented 7 years ago

other than notes in https://github.com/yoshuawuyts/multipart-read-stream/issues/6 I think it's super nice

on a side note, re merry.error take a look at https://github.com/apparatus/mu/blob/master/packages/mu-error/index.js - there might be some ideas in there that appeal

yoshuawuyts commented 7 years ago

@davidmarkclements ohh, thanks for the link - there's def a lot more going on given that it's for distributed systems. Could you perhaps elaborate a little on which ideas would be neat for merry? Definitely feel there's room for improvement :grin:

davidmarkclements commented 7 years ago

Well couple of things

  1. The boom module has some great concepts when it comes to handling and propagating HTTP errors to HTTP clients (mu-error wraps boom) - boom objects are ultimately instances of Error object - in particular 500+ errors messages are automatically redacted - mu-error adds a dev option to re-expose them.
  2. Error serialization - when you stringify an Error object you don't get the message or the stack (although with pino you do, so for most cases this may already be solved) - mu-error solves this in a performant way by piggybacking on the JSON stringify loop (essentially using toJSON)
  3. Conversion of error-like objects to legit. Error objects without copying properties - this can be a nice way to enforce an error creation pattern (that all errors are instances of Error) whilst providing a convenient API for adding props to Errors - e.g. instead of
var err = new Error('not found')
err.code = 404

You could do:

var err = merry.error({message: 'not found', code: 404})

And this would morph the input object into an Error object (avoiding property copying is much better perf wise)

lrlna commented 7 years ago

@yoshuawuyts really like the multipartRoute actually 🎉

@davidmarkclements i think merry.error currently does this:

var err = merry.error(400, 'error parsing json', e)

but i am really into setting it up as an Error object instead tbh; much cleaner.

switch err discussion to #30 tho?

yoshuawuyts commented 7 years ago

Multipart implemented; closing issue :sparkles: