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 #99

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support Http2 #99

wants to merge 4 commits into from

Conversation

sogaani
Copy link

@sogaani sogaani commented Jul 30, 2018

Support http2 to enable express tests with http2.
expressjs/express#3390.

@sogaani
Copy link
Author

sogaani commented Aug 9, 2018

@dougwilson ping

.travis.yml Outdated
@@ -17,6 +19,7 @@ before_install:
# Setup Node.js version-specific dependencies
- "test $TRAVIS_NODE_VERSION != '0.8' || npm rm --save-dev istanbul"
- "test $(echo $TRAVIS_NODE_VERSION | cut -d. -f1) -ge 4 || npm rm --save-dev eslint"
- "test -z $(echo $HTTP2_TEST) || npm install --only=dev https://github.com/sogaani/express.git#initial-support-http2 https://github.com/sogaani/supertest.git#http2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please only install deps from npm.

Copy link
Author

Choose a reason for hiding this comment

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

Express and this PR has circular dependency. Would you allow me to add http2 enabled express package like express-http2 for npm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Express.js does not have a dependency on this module. Not sure what you mean there is a circular dependency.

Copy link
Author

Choose a reason for hiding this comment

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

Express use cookie-session for test, and cookie-session depend on cokkies.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the tests in this module won't work with the published version of Express?

Copy link
Author

@sogaani sogaani Aug 9, 2018

Choose a reason for hiding this comment

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

Yes.
No.
I met following errors with the published version of Express.

  1) Express should set cookies:
     Uncaught TypeError: Cannot read property 'readable' of undefined
      at IncomingMessage._read (_http_incoming.js:104:19)
      at IncomingMessage.Readable.read (_stream_readable.js:442:10)
      at IncomingMessage.read (_http_incoming.js:96:15)
      at resume_ (_stream_readable.js:822:12)
      at _combinedTickCallback (internal/process/next_tick.js:138:11)
      at process._tickCallback (internal/process/next_tick.js:180:9)

@sogaani sogaani mentioned this pull request Aug 28, 2018
5 tasks
@dougwilson dougwilson added the pr label Sep 9, 2018
@dougwilson dougwilson added this to the 0.8 milestone Sep 10, 2018
@sogaani
Copy link
Author

sogaani commented Sep 17, 2018

I fixed PR installing deps only from npm.

@dougwilson dougwilson removed this from the 0.8 milestone Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants