search

ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.56k stars 1.33k forks source link

Does superagent support ALPN for http2 ? #1754

Open Louis-Tian opened 1 year ago

Louis-Tian commented 1 year ago

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 commented 1 year ago

Ok, searching through the pull request and found the original pull request https://github.com/visionmedia/superagent/pull/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 commented 1 year ago

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 commented 1 year ago

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

Louis-Tian commented 1 year ago

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 commented 1 year ago

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