Skip to content
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

handle undefined statusCode with an error #3358

Conversation

ghinks
Copy link

@ghinks ghinks commented Jul 7, 2017

description

We can bettor handle the error case when an undefined is sent as the status code in the response as it currently goes down to the node layer and does not give a clear error.

var express = require('express');
var app = express();

app.get('/', function (req, res) {
  //res.status(undefined).send('not ok');
  // or
 res.sendStatus(undefined);
});

app.listen(3000);

If the status code is undefined when you call

res.status(undefinded).send('');

you get an error OF COURSE. But I think we could handle this case a bit more clearly than the stack trace allows.

instructions to reproduce

npm start

then hit the end point

curl -X GET http://localhost:3000/ 

The following is what you currently see if you send an undefined statusCode in a response.

> express-status-undefined-error@1.0.0 start /Users/ghinks/dev/doodles/express-status-undefined-error
> node server.js

TypeError: Cannot read property 'toString' of undefined
    at ServerResponse.writeHead (_http_server.js:190:44)
    at ServerResponse._implicitHeader (_http_server.js:157:8)
    at ServerResponse.OutgoingMessage.end (_http_outgoing.js:548:10)
    at ServerResponse.send (/Users/ghinks/dev/doodles/express-status-undefined-error/node_modules/express/lib/response.js:211:10)
    at /Users/ghinks/dev/doodles/express-status-undefined-error/server.js:9:25
    at Layer.handle [as handle_request] (/Users/ghinks/dev/doodles/express-status-undefined-error/node_modules/express/lib/router/layer.js:95:5)
    at next (/Users/ghinks/dev/doodles/express-status-undefined-error/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/Users/ghinks/dev/doodles/express-status-undefined-error/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/Users/ghinks/dev/doodles/express-status-undefined-error/node_modules/express/lib/router/layer.js:95:5)
    at /Users/ghinks/dev/doodles/express-status-undefined-error/node_modules/express/lib/router/index.js:281:22

I think that something with

Error: statusCode was not a number

is maybe an improvement.

@dougwilson
Copy link
Contributor

I believe this is a duplicate of #3143 . @ghinks can you confirm / deny ?

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together! Besides the comment to check out if this is similar to a previous PR, if you believe it is not, please feel free to update the PR with tests for the new validations.

@dougwilson
Copy link
Contributor

I tried your example, and I'm not sure what is happening, but I'm seeing some very different things that what you reported. Here are the two biggest differences:

  1. I ran your example against the current version of Express and the output is RangeError: Invalid status code: 0 not the error you posted.
  2. I ran your example against the modified version of Express in the PR but still saw the same error; the code code and error you added was not triggered at all.

@ghinks
Copy link
Author

ghinks commented Jul 7, 2017

this is not quite a duplicate of #3143
So I shall continue with this add tests and get it working as you have suggested.

@ghinks
Copy link
Author

ghinks commented Jul 7, 2017

ok, what is your current version of express haha. I have an example at https://github.com/ghinks/express-status-code-error-example.git
but my current version is 4.15.3. Are you using another branch I can pull ?
Sincere thanks for spending your time on Express. I do want to help and hope that I am being useful here.

@ghinks
Copy link
Author

ghinks commented Jul 8, 2017

Ok, I understand the issue now.
I am using node 4.2.4 in production which gives the following error for

res.status(undefined).send('not ok');

<pre>TypeError: Cannot read property &#39;toString&#39; of undefined<br>...

where as newer node versions ( I tested with 6.8.0 )

give

RangeError: Invalid status code: 0

which is a lot more helpful. So node core fixed what I wanted to do in express which was give a meaningful error when an undefined was sent.

@ghinks ghinks closed this Jul 8, 2017
@dougwilson
Copy link
Contributor

Ah, sorry, didn't think Node.js used to have a bad message. Yes, I was using 6.11.0 and Express 4.15.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants