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

Does superagent support ALPN for http2 ? #1754

Open
Louis-Tian opened this issue Nov 30, 2022 · 5 comments
Open

Does superagent support ALPN for http2 ? #1754

Louis-Tian opened this issue Nov 30, 2022 · 5 comments

Comments

@Louis-Tian
Copy link

The documentation on http2 currently reads
https://github.com/visionmedia/superagent/blob/92b1435f4fd02193b12c6cf14f6ef4ca02b22bd6/docs/index.md?plain=1#L98
This sounds like the superagent will try to force the http2 only when the http2() method is called. When a request is made without the http2() method superagent will by default use the http2 and callbacks to http1 if the server does not support http2 (presumably using the ALNP?).

But at the moment (tested with superagent 8.0.0) when making a request using a superagent without the http2() to a server created by http2.createSecureServer without the 'allowHTTP1' options will result in an 'Unknown ALPN Protocol, expected h2 to be available' error on the server side.

According to https://nodejs.org/api/http2.html#event-unknownprotocol, this means the client

either does not send an ALPN extension or sends an ALPN extension that does not include HTTP/2 (h2).

Does this contradict what the superagent's documentation seems to be implying? Some clarification on the expected behavior will be very helpful. Thanks

@Louis-Tian
Copy link
Author

Ok, searching through the pull request and found the original pull request #1399 by @sogaani which introduced the support for http2 back in 2018. He mentioned explicitly that ALNP is not supported in that pull request. I couldn't locate any further pull requests that made any further improvement on that. If that is the case, then I think the current document needs to be updated. As it stands now, it is a bit misleading. Thoughts?

@Louis-Tian
Copy link
Author

If one must always call http2() explicitly to make any http2 request, then there is a need for the Agent class to be augmented with a similar method that changes the default HTTP protocol to http2, otherwise, it because verbose and tedious having to add '.http2()' for every single request.

@titanism
Copy link
Collaborator

Make a wrapper in your project that wraps calls with http2 invocation or use agent()?

@Louis-Tian
Copy link
Author

Louis-Tian commented Nov 30, 2022

I think the usage of http2 will be, if not already, common enough and shall be supported by the library directly. Of course, wrapping the library is always an option but the whole point of having a library like superagent is to make common tasks simpler, otherwise why not just use the node built-in https and http2 modules?

As with the agent() usage, that's exactly what I am suggesting in my second comment. Currently, you can't do something like agent().http2() to set the default protocol. It's neither listed in the documentation as an available method (https://visionmedia.github.io/superagent/#agents-for-global-state) nor is it actually a defined property on the returning value from agent() when I tested locally on 8.0.0.

@titanism
Copy link
Collaborator

titanism commented Dec 5, 2022

We'd welcome a PR to add this to agent usage.

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

No branches or pull requests

2 participants