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

Don't assume res._header exists #213

Merged
merged 2 commits into from
Sep 19, 2016
Merged

Don't assume res._header exists #213

merged 2 commits into from
Sep 19, 2016

Conversation

SpaceK33z
Copy link
Contributor

res._header is an internal implementation detail. Hence, some implementations of the http server do not set this. You can't blame them for this, because this is a hack.

node-spdy is an example.

`res._header` is an internal implementation detail. Hence,
some implementations of the http server do not set this.
[node-spdy](https://github.com/indutny/node-spdy) is an example.
@SpaceK33z SpaceK33z changed the title Don't assume res._header exist Don't assume res._header exists Sep 19, 2016
@@ -120,7 +120,7 @@ exports.GenericApp = class GenericApp
log_request: (req, res, data) ->
td = (new Date()) - req.start_date
@log('info', req.method + ' ' + req.url + ' ' + td + 'ms ' +
(if res.finished then res._header.split('\r')[0].split(' ')[1] \
(if res.finished and res._header then res._header.split('\r')[0].split(' ')[1] \
Copy link
Contributor

@brycekahle brycekahle Sep 19, 2016

Choose a reason for hiding this comment

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

This will log (unfinished) if res._header does not exist, even if res.finished is true. Can you fix that?

Copy link
Contributor

@brycekahle brycekahle left a comment

Choose a reason for hiding this comment

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

Fix my comment about the logging and we can get this merged.

@SpaceK33z
Copy link
Contributor Author

SpaceK33z commented Sep 19, 2016

@brycekahle, it seems like the internal _header method was used to get the status code. However, this can simply be done with res.statusCode. This is supported since node v0.4.0 and used everywhere throughout this package, so I don't know why this hack was still used. Could you take a look again?

@brycekahle brycekahle merged commit ac7dd95 into sockjs:master Sep 19, 2016
@brycekahle
Copy link
Contributor

Thanks!

@SpaceK33z SpaceK33z mentioned this pull request Sep 20, 2016
@SpaceK33z SpaceK33z deleted the header-check branch September 20, 2016 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants