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

Add native HTTP/2 support #1489

Merged
merged 1 commit into from Nov 2, 2017
Merged

Add native HTTP/2 support #1489

merged 1 commit into from Nov 2, 2017

Conversation

hekike
Copy link
Member

@hekike hekike commented Sep 12, 2017

Please provide feedback in the comment section about this approach.

Issues

Related:

Add HTTP/2 support to restify

Changes

  • Adds HTTP/2 support to server
  • Uses patch method to decorate Request and Response objects
  • Example for HTTP/2 server with restify

TODO

How to run

At the moment you need to build Node.js with the latest master and using the --expose-http2 flag to run this branch.

@hekike hekike changed the title feat(http2): add HTTP/2 support [WIP] A native HTTP/2 support Sep 12, 2017
@hekike hekike changed the title [WIP] A native HTTP/2 support [WIP] Add native HTTP/2 support Sep 12, 2017
@retrohacker
Copy link
Member

image

@yunong
Copy link
Member

yunong commented Sep 12, 2017

🎉 I've assigned a few reviewers to look at this, but this is amazing work so far.

@hekike
Copy link
Member Author

hekike commented Oct 25, 2017

@DonutEspresso @yunong @jclulow @retrohacker Node.js v8.8.0 is out which means we have everything that's needed to ship this feature, see changelog:

  • no more conflict with the path getter
  • http2 is now exposed by default without the need for a flag
  • However http2 stability is still experimental in the Node core

I rebased this PR with the master and added a simple test case.
Do you want to wait more or can we ship it?

@yunong
Copy link
Member

yunong commented Oct 26, 2017

Looks great. What do you think about adding some documentation to the home page about how we support HTTP/2, but that it's still experimental? I think this would give us tremendous PR!

@yunong
Copy link
Member

yunong commented Oct 26, 2017

What happens if we're on a version of Node that doesn't support http2, does the current code handle this path?

@hekike
Copy link
Member Author

hekike commented Oct 27, 2017

@yunong I added an assert to the server.js http2 option.

The http2 related tests will be skipped with older versions:

HTTP2 module is not available
Node.js version >= v8.8.8 required, current: 6.9.4

OK: 0 assertions (4ms)

@hekike
Copy link
Member Author

hekike commented Oct 27, 2017

@yunong regarding docs and landing I can create some hands-on guide for restify's http2 support, some ideas:

@yunong
Copy link
Member

yunong commented Oct 27, 2017

@hekike That blog post is great, maybe we can link to some articles instead of having to maintain HTTP/2 specific documentation.

I was more thinking about just adding docs to say that we support HTTP/2 now, with some pointers to code snippets or examples, which you already have added.

@hekike
Copy link
Member Author

hekike commented Oct 30, 2017

@yunong I agree. What do you think, should we link the articles it in this PR?
I believe we could focus on the marketing after the feature is landed.

@yunong
Copy link
Member

yunong commented Oct 31, 2017

Yeah we should land and then follow up with some docs :)

@hekike hekike force-pushed the feat/http2 branch 2 times, most recently from fcdb387 to 3072e38 Compare November 1, 2017 10:37
@hekike
Copy link
Member Author

hekike commented Nov 1, 2017

Breaks because of: #1545

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.

None yet

3 participants