feat: Node.js HTTP/2 Handler #317
Conversation
Codecov Report
@@ Coverage Diff @@
## master #317 +/- ##
=========================================
Coverage ? 89.75%
=========================================
Files ? 239
Lines ? 5545
Branches ? 1136
=========================================
Hits ? 4977
Misses ? 568
Partials ? 0
Continue to review full report at Codecov.
|
trivikr
force-pushed
the
http2-handler
branch
2 times, most recently
from
2bf48be
to
c062481
Compare
|
The code in this PR is ready for review, unit tests are pending |
trivikr
changed the title
trivikr
force-pushed
the
http2-handler
branch
3 times, most recently
from
28d5099
to
320d4b3
Compare
|
|
||
| req.on("error", reject); | ||
|
|
||
| const { connectionTimeout } = this.httpOptions; |
There was a problem hiding this comment.
Maybe it's out of the scope of the PR but maybe you have answer to this: we support a per request httpOptions setting like here. Where does it supplied to http handlers?
| resolve(httpResponse); | ||
| }); | ||
|
|
||
| req.on("error", reject); |
There was a problem hiding this comment.
We might need to handle abort event: https://nodejs.org/docs/latest/api/http2.html#http2_event_aborted
Same as frameError, trailers, wantTrailers
There was a problem hiding this comment.
Added listeners for aborted and frameError events in c49ce80
| [constants.HTTP2_HEADER_PATH]: queryString | ||
| ? `${path}?${queryString}` | ||
| : path, | ||
| [constants.HTTP2_HEADER_METHOD]: method |
There was a problem hiding this comment.
According to the doc, there are other headers available(:authority, :scheme). These information should have been specified when you create a connection. So what are they used for? What if these headers are conflict with then endpoint you used to create a connection?
UPDATE: I believe they are only useful in response headers. But we should confirm
|
As TravisCI times out for this PR as described in #346, here's coverage report of |
There was a problem hiding this comment.
Can you also add a test suite around the streaming response and 100-continue header? Otherwise it looks good to me. Btw, we have a bunch of commented tests in http1 handler test because of Jest issue.
|
Followed up in PR #414 for |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue #, if available:
N/A
Description of changes:
Work in Progress, posting a Draft Pull Request for early look
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.