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

Support http2 #36

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
6 changes: 6 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,9 @@ script:
- "test -z $(npm -ps ls eslint ) || npm run-script lint"
after_script:
- "test -e ./coverage/lcov.info && npm install coveralls@2 && cat ./coverage/lcov.info | coveralls"
matrix:
include:
- node_js: "9.5"
env: HTTP2_TEST=1
- node_js: "8.9"
env: HTTP2_TEST=1
22 changes: 21 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,36 @@ function typeis (value, types_) {
* or `content-length` headers set.
* http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.3
*
* A http/2 request with DataFrame can have no `content-length` header.
* https://httpwg.org/specs/rfc7540.html
*
* A http/2 request without DataFrame send HeaderFrame with end-stream-flag.
* If nodejs gets end-stream-flag, then nodejs ends readable stream.
* https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L301
*
* @param {Object} request
* @return {Boolean}
* @public
*/

function hasbody (req) {
return req.headers['transfer-encoding'] !== undefined ||
return (ishttp2(req) && !req.stream._readableState.ended) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Some testing seems like this may have an issue. If you add a req.on('data' listener and read the body, this then returns false for http/2 but true for http/1. The current interface it is expected to be true for both, as hasBody determines if the request has body, regardless of it you happened to have read it already or not.

Copy link
Author

Choose a reason for hiding this comment

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

I checked and happens an issue you said. I address this issue and add a test.

req.headers['transfer-encoding'] !== undefined ||
!isNaN(req.headers['content-length'])
}

/**
* Check if a request is a http2 request.
*
* @param {Object} request
* @return {Boolean}
* @public
*/

function ishttp2 (req) {
return req.httpVersionMajor === 2
}

/**
* Check if the incoming request contains the "Content-Type"
* header field, and it contains any of the give mime `type`s.
Expand Down
50 changes: 46 additions & 4 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,55 @@ describe('typeis.hasBody(req)', function () {
assert.strictEqual(typeis.hasBody(req), true)
})
})

describe('http2 request', function () {
it('should not indicate body', function () {
var req = {
headers: {},
stream: {
_readableState: {
ended: true
}
},
httpVersionMajor: 2
}
assert.strictEqual(typeis.hasBody(req), false)
})

it('should indicate body', function () {
var req = {
headers: {},
stream: {
_readableState: {
ended: false
}
},
httpVersionMajor: 2
}
assert.strictEqual(typeis.hasBody(req), true)
})
})
})

function createRequest (type) {
return {
headers: {
'content-type': type || '',
'transfer-encoding': 'chunked'
if (process.env.HTTP2_TEST) {
return {
headers: {
'content-type': type || ''
},
stream: {
_readableState: {
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point if we are depending on Node.js internals for the state, we need to test against the actual Node.js implementation instead of a simple mock.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either that or there is specific documentation in Node.js documenting this exact behavior, though I didn't see that anywhere. Without one of these we won't know when Node.js changes it's behavior as our tests won't fail.

Copy link
Author

Choose a reason for hiding this comment

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

I will fix tests tomorrow.

ended: false
}
},
httpVersionMajor: 2
}
} else {
return {
headers: {
'content-type': type || '',
'transfer-encoding': 'chunked'
}
}
}
}