jshttp / jshttp.github.io

https://jshttp.github.io/
MIT License
47 stars 3 forks source link

Standard error interface #35

Closed jonathanong closed 9 years ago

jonathanong commented 9 years ago

Would be cool if these modules and any related ones have a common error syntax. Would be cool to have err.code matching node or something as well as .status if applicable

Raynos commented 9 years ago

I've recently written an error serializer for our json APIs ( https://gist.github.com/Raynos/91cab673906aebd04eac )

I settled on the interface

type TypedError : {
  message: String,
  statusCode?: Number,
  type?: String,
  stack?: String,
  expected?: Boolean,
  messages?: Array<String>
}

The message field is the standard error message.

The statusCode field is a number that represents the http status code thats applicable for this error, this is optional and defaults to 500.

The type field is a static string, it's like err.code but a string is more readable then a static number. This field allows for the message field to be dynamic and means you can detect what kind of error something is without doing a regular expression match.

The stack field is the standard error stack

The expected boolean allows you to encode whether this an expected or unexpected error. A body parser might run into an unexpected net IO error which means it should be treated as a unexpected 500 error or might run into an expected JSON parse error in which case it should be treated as a 400.

In production you want to only serialize the message field for expected 4xx errors and serialize the message field as Unexpected Server Error for unexpected 500 fields. Serializing arbitrary unexpected error message leaks implementation details and creates vulnerabilities

The messages field is slightly opinionated. When dealing with body validation errors you might want to send multiple validation messages. This could be done with a comma seperated message field or with an array of messages.

I don't care that much about field names, however you generally want the following information

jonathanong commented 9 years ago

so i really like stripe's error: https://stripe.com/docs/api#errors

dougwilson commented 9 years ago

expected - not sure if the name itself makes sense. koa uses .expose , which i think is a better name. i agree with the 4xx vs 5xx stuff.

personally I like .public :)

rlidwka commented 9 years ago

How about plain old Error object with status property on it?

Something like finalhandler can capture those... with syntax like:

var err = new Error('blah not found')
err.status = 404
res.end(err)
dougwilson commented 9 years ago

Also, as for deciding if the message should be written to the response or not, there is an upcoming message option on a future finalhandler that can determine stuff like that from the user: https://github.com/expressjs/finalhandler/tree/1.x#optionsmessage ; the default is to always hide the real message right now. we can always determine a good way :)

rlidwka commented 9 years ago

Imho, it should be an extension of an Error object. People are so used to dealing with it, so changing interface is a bad idea. I'm even thinking about shadowing the real Error object to get syntax highlighting and stuff.

So here is what I came up with so far:

var Error = require('./error')
throw Error('message').http(404)

/*
Error 404: message
    at new Error (/tmp/error.js:7:16)
    at Error (/tmp/error.js:5:40)
    at Object.<anonymous> (/tmp/index.js:6:7)
    at Module._compile (module.js:456:26)
*/

// with a possibility of future extensions like:
// throw Error('message').fs('ENOENT')

Implementation:

module.exports = Error

function Error(message, a, b) {
  var err = new global.Error(message, a, b)
  err.__proto__ = Error.prototype
  return err
}

require('util').inherits(global.Error, Error)

Error.prototype.http = function(status) {
  this.status = status
  return this
}

Error.prototype.toString = function() {
  var ret = 'Error'
  if (Number(this.status) >= 200 && Number(this.status) < 600) {
    ret += ' ' + this.status
  }
  ret += ': ' + this.message
  return ret
}
dougwilson commented 9 years ago

If the resulting object is err, then Object.prototype.toString.call(err) === '[object Error]'. As long as the errors objects meet that requirement, I don't particularly care.

rlidwka commented 9 years ago

@dougwilson , I need a quick sanity check here: https://gist.github.com/rlidwka/0ad094386f60f93318bf

I was looking for a way to subclass Errors properly, but it looks like I just found a better way to subclass everything.

rlidwka commented 9 years ago

And here's proposed module: https://github.com/rlidwka/error, let me know if you like the approach.

No idea if it'll be useful, but I definitely going to switch to it from my custom new HTTPError({message: 'blah', code: 404}) crap.

Raynos commented 9 years ago

I have a similar helper for creating errors ( https://github.com/Raynos/error/blob/master/typed.js ). This is actually the error module on npm.

dougwilson commented 9 years ago

Both of those methods work :) I wonder which is faster, though. I would think __proto__ is going to be slower, but idk.

rlidwka commented 9 years ago

I wonder which is faster, though. I would think __proto__ is going to be slower, but idk.

It's full-blown example, and you don't really need to use __proto__. Basically, I was suggesting to create classes like this:

function Class() {
  var self = Object.create(Class.prototype)
  self.foo = 1
  return self
}

Benchmarks are surprising though. It's 30% faster on node 0.10 and 11% slower on node 0.11. :D

jonathanong commented 9 years ago

should i push for http-error or http-errors?

jonathanong commented 9 years ago

god i can't find an appropriate name

jonathanong commented 9 years ago

https://github.com/jshttp/http-errors