closes #107: Swagger should not override error handling.

[jsdevel]

No description provided.

[fehguy]

should this move into the develop branch?

[jsdevel]

@fehguy this would create a breaking change so I can understand your desire there. How willing are you to bump the major version for this alone? I'd like to use this sooner rather than later.

[jsdevel]

@fehguy if we need to make updates to the 2.x.x branch, we could always create a branch for that based on the commit prior to this one.

[tracker1]

As mentioned in #130 I'd update the example to...

throw {status:400, message:'missing required foo'}

And the error handler...

function(err,req,res,next) {
  res.send(err.status || 500, {error:err})
}

---

oh yeah, and bump at least the minor to the next odd number in the package.json

[tracker1]

Would also suggest considering adding the changes to Swagger.prototype.errors in #130 .. or removing Swagger.prototype.errors altogether.

[jsdevel]

@tracker1 I updated the sample to your liking.

This PR really just removes the error handling from swagger and places it back in the hands of express. Given that, I think changes to Swagger.prototype.errors warrant another PR. Thoughts?

[hillct]

While I'm not in a place where I can test this, the code change looks solid (on visual inspection).

I wonder if addition of a flag to revert back to the old error handling behavior, such that this would no longer be a breaking change, is worthwhile? I have very mixed feelings about this as the code un-cleanliness such a flag would introduce offends me almost to the point where I'm pained to suggest that kind of thing. Another option would be to implement the old-style error handling as a separate module such that existing swagger-node-express users would not be stranded without and upgrade path, but would merely be required to import the separate error handler.

[jsdevel]

@hillct I'm with you on the flags.

I don't see too much of a difference between express's default error handler and the one swagger provides. The pain point with this breakage is that calls to swagger.setErrorHandler will throw an error. Perhaps a console.error within the body of swagger.setErrorHandler with an instruction to use app.use instead would be better?

[enterline]

maybe a swagger.unsetErrorHandler function that sets this.errorHandler to null. Then in addMethod do the try-catch if this.errorHandler !== null otherwise just do the callback. That way it is not a breaking change.

[jsdevel]

@fehguy I added .setErrorHandler() back to swagger. Calls to it will now log a warning in the console instructing the user to use standard express middleware instead. I've also added a note in the README about this change for 2.1.0 and bumped the version in package.json.

Can you create a 2.0.x branch prior to merging this? Doing so will allow us to handle issues that arise for that version. Thanks!

[fehguy]

hi @jsdevel this is done https://github.com/wordnik/swagger-node-express/tree/2.0.x

[timruffles]

LGTM!

[jsdevel]

Thanks @fehguy @timruffles ! I'm looking forward to getting this merged and published so we can start referencing it ;)

[fehguy]

Note, this commit produces a 2.1.x version of the swagger module, and has breaking changes with 2.0.x!

[fehguy]

pushed 2.1.0 to npmjs.org

[jsdevel]

Awesome @fehguy! Thank you 😄