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 transparent HTTP/2 with spdy module #70

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

ishitatsuyuki
Copy link
Contributor

This adds an opt-out based HTTP/2 support based on node-spdy. It's tested on many Node version including 0.10, so there shouldn't be much risk of breakage.

@jankeromnes
Copy link
Collaborator

jankeromnes commented Mar 23, 2018

Nice! Very cool and surprisingly small change! Thanks a lot.

Would you mind adding a quick sanity check for this? I don't think we need to call all test-api.js tests on your new SPDY server (also the HTTP 1.1 check would fail), but maybe you could create a new file like /test/test-secure.js? (You can take inspiration from /test/test-api.js, e.g. for imports and the port-choosing boilerplate at the bottom, and then please add that file to make test, or to npm test directly).

@espadrine
Copy link
Owner

Very nice indeed!
Could you rebase it?

@espadrine
Copy link
Owner

espadrine commented Apr 4, 2018

It seems like SPDY is dying though: Chrome killed it in 2015, Firefox in 2016, so support is lacking. The npm package hasn't seen an update in a year.

Could you add support for HTTP/2 instead maybe? @jankeromnes Maybe you could help with the tests?

@ishitatsuyuki
Copy link
Contributor Author

@espadrine Although this package is called spdy, it just supports H2 as well.

@espadrine
Copy link
Owner

espadrine commented Apr 4, 2018

@ishitatsuyuki My main worry is that the spdy package is likely to be deprecated in favour of the built-in HTTP/2 Node.js module (if it is not already so in the minds of the makers).

So we may not receive security updates or other important update (or any update at all).

@ishitatsuyuki
Copy link
Contributor Author

@espadrine What I can say is that the official H2 module doesn't expose the Server class we have to use and thus cannot be inherited (apart from that, it's ABI isn't guaranteed to be stable). I'm aware that it's not well maintained, but it's still too early to switch to the official impl either.

@espadrine
Copy link
Owner

Good points! We'll figure out solutions to the http2 ordeal when it comes out of experimental status.

@espadrine espadrine merged commit cd6851f into espadrine:master Apr 4, 2018
@espadrine
Copy link
Owner

@jankeromnes Would you be interested in contributing related tests as a follow-uip?

@jankeromnes
Copy link
Collaborator

@espadrine I won't have time to look into this anytime soon, sorry. Thanks for merging! And thanks @ishitatsuyuki for implementing this! 💯

@espadrine
Copy link
Owner

@ishitatsuyuki Hello! The SPDY module now crashes because of this 6-month old bug: spdy-http2/node-spdy#350

I am considering reverting this patch. Do you know a better solution by any chance?

@ishitatsuyuki
Copy link
Contributor Author

It seems Node have implemented HTTP/2 as a reasonable built-in. I will do a rewrite this weekend.

@ishitatsuyuki
Copy link
Contributor Author

ishitatsuyuki commented May 10, 2019

Actually no, please revert this. Node ecosystem seems to have a number of bugs with the HTTP/2 module due to the class usage. (Additionally, sc uses inheritance which does not work with http2 because the SecureServer class is not exposed.)

They're listed in expressjs/express#3730, for reference.

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.

3 participants