-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
add logging for error handling edge cases #742
Conversation
@@ -42,7 +43,10 @@ function sendJSONResponse(req, res, next) { | |||
if( err instanceof es.errors.RequestTimeout ){ statusCode = Math.max( statusCode, 408 ); } | |||
else if( err instanceof es.errors.NoConnections ){ statusCode = Math.max( statusCode, 502 ); } | |||
else if( err instanceof es.errors.ConnectionFault ){ statusCode = Math.max( statusCode, 502 ); } | |||
else { statusCode = Math.max( statusCode, 500 ); } | |||
else { | |||
logger.warn( 'unknown geocoding error object:', err.constructor.name, err ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought we were going to check for existance of err.contructor here? seems a bit optimistic to assume it's there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a think about it and the if( err instanceof Error ){
check above guarantees that err.constructor.name
will always exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var err = new Error()
err.constructor.name;
"Error"
delete err.constructor
true
err.constructor.name;
"Error"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function MyError(){}
MyError.constructor = Error.Constructor;
var err = new MyError()
err.constructor.name;
"MyError"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason.. even with a typo the above works :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, it works for any function:
function test(){}
test.constructor.name;
"Function"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and because javascript; everything apparently
(99).constructor.name
"Number"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh javascript...
(NaN).constructor.name
"Number"
if( res.statusCode < 400 ){ res.status(500); } | ||
res.json({ error: typeof error === 'string' ? error : 'internal server error' }); | ||
if( res.statusCode < 400 ){ | ||
logger.info( 'status code changed from', res.statusCode, 'to 500' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need spaces after from
and before to
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log( "doesn't", "it", "add", "them", "for", "me?" );
doesn't it add them for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I learn something new about javascript every day!
Superseded by #751 |
add logging for error handling edge cases