Fix: emit restifyError event even for router errors #1420
Conversation
lib/server.js
Outdated
|
|
||
|
|
||
| /** | ||
| * TODO: this is relevant for CORS only. Should move this out eventually to a |
There was a problem hiding this comment.
Should we scrub TODOs from the codebase in favor of GitHub issues?
There was a problem hiding this comment.
👍 on this idea! I think it might be useful to have a new label then, something like.. internal for these non user facing clean up tasks/refactoring.
lib/server.js
Outdated
| res.send(200); | ||
| ok = true; | ||
| if (err.statusCode === 404 && req.method === 'OPTIONS' && req.url === '*') { | ||
| return true; |
There was a problem hiding this comment.
Nice! This is much cleaner! 😄
We could even return the equality:
return err.statusCode === 404 && req.method === 'OPTIONS' && req.url === '*'
| var r = route ? route.name : null; | ||
|
|
||
| if (err) { | ||
| // TODO: if its a 404 for OPTION method (likely a CORS | ||
| // preflight), return OK. This should move into userland. |
There was a problem hiding this comment.
Why would we return 200 on an invalid preflight route?
There was a problem hiding this comment.
I updated the TODO in the optionsError() method to reflect what's going on here. This behavior isn't actually new - just moved it out of the optionsError() method. Here's the comment I added:
/**
* returns true if the router generated a 404 for an options request.
* TODO: this is relevant for CORS only. Should move this out eventually to a
* userland middleware? This also seems a little like overreach, as there is no
* option to opt out of this behavior today.
lib/server.js
Outdated
| @@ -889,73 +885,18 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { | |||
|
|
|||
| if (arg) { | |||
| if (arg instanceof Error) { | |||
There was a problem hiding this comment.
Not part of this PR, but doesnt arg instanceof Error === true imply arg? This second check seems redundant and adds a level of indentation. Same with typeof
There was a problem hiding this comment.
arg can also be a boolean, or more specifically, a value of false which tells restify to stop executing the handler chain.
lib/server.js
Outdated
| // setting this flag will allow us to to not call cb() | ||
| // automatically later in this function. | ||
| emittedError = self._emitErrorEvents(req, res, route, arg, | ||
| function emitErrorsDone() { |
There was a problem hiding this comment.
Did we lint this? It looks like this should have an extra level of indentation.
There was a problem hiding this comment.
Yep! Lint doesn't complain because function invocations don't have to be indented even if they are > 1 line.
I actually prefer this style, as it gives us an extra 4 spaces for this last arg. Open to suggestions on how to better do this and still keep within 80cols though.
lib/server.js
Outdated
| * emit error events when errors are encountered either while attempting to | ||
| * route the request (via router) or while executing the handler chain. | ||
| * @private | ||
| * @function emitErrorEvents |
There was a problem hiding this comment.
This is missing the _ prefix
|
|
||
| // while async listeners are firing, return a boolean indicating whether or | ||
| // not we had any listeners to fire. | ||
| return (errEvtNames.length > 0) ? true : false; |
There was a problem hiding this comment.
This block of code is super clever! I dig it! 👏 poetry 👏
DonutEspresso
force-pushed
the
fix-emit-err
branch
from
724d21f
to
9823469
Compare
|
Did a little clean up of the _run function (scope creep!) but also added a ton of comments to better breakdown what's going on. |
DonutEspresso
force-pushed
the
fix-emit-err
branch
from
9823469
to
869a41c
Compare
| // flush the err. | ||
| var errResponse = function errResponse(err) { | ||
| return self._emitErrorEvents(req, res, null, err, function () { | ||
| res.send(err); |
There was a problem hiding this comment.
all res.send() requires to check res.headersSent otherwise node will throw Can't set headers after they are sent.
Fixes #1419
This also removes the old FormatterError used in async formatters, which is no longer applicable.