Closed andyleclair closed 2 years ago
Would it be better to expose the protocol instead of the module? Something like get_protocol(Mint.HTTP.t()) :: :"http1.1" | :http2
?
I see that @ericmj suggested the module-based solution in https://github.com/elixir-mint/mint/issues/263. Any reason not to go with the protocol instead?
In the scenario from #263 I think it's better to return the connection module since they want to check if they can call functions on a specific module.
There may be a use case to get the protocol as well but that would need a different API I think since we support HTTP 0.9, 1.0, 1.1, and 2.0. Maybe protocol_version(t()) :: {pos_integer(), pos_integer()}
?
@ericmj yes I like the protocol + version tuple. Can't folks map the protocol to the HTTP module? Wondering if we should keep the module internal, that's all :)
I don't think we need to keep it internal, Mint.HTTP1
and Mint.HTTP2
are public documented modules.
We can add protocol(t()) :: {atom(), pos_integer(), pos_integer()}
instead but then we also need to document that you can call Mint.HTTP1
functions with a connection struct returned from Mint.HTTP.connect()
if Mint.HTTP.protocol()
returns {:http, 0 | 1, _}
. It feels more complicated when we can instead return the module directly.
?
I’m worried about the proxy too here. If we return the module it's not guaranteed that you can call functions on that, is it? Because it could be a proxy conn?
For our purposes, we're looking to differentiate in order to fix a bug with transfer-encoding: chunked
(see also: https://github.com/appcues/mojito/pull/87). We don't need to call functions on the module, the module name was good enough
Because it could be a proxy conn?
Yeah, this would break on a proxy conn.
@andyleclair Sorry for the back and forth, but could you add a protocol(t()) :: {atom(), pos_integer(), pos_integer()}
function instead?
hey no problem! I'm glad to help. thank you both for your time (and for considering this PR)
From my understanding, I'd need a request to determine the full protocol from the conn, right? Obviously for HTTP2, it's just {:http, 2, 0}
but to tell 0.9
from 1.0
from 1.1
we'd need a request body as the version is returned from the server
From my understanding, I'd need a request to determine the full protocol from the conn, right? Obviously for HTTP2, it's just {:http, 2, 0} but to tell 0.9 from 1.0 from 1.1 we'd need a request body as the version is returned from the server
Ugh, good point.
In that case I am leaning towards returning :http1 | :http2
. Alternatively return the module names, but add support for %UnsafeProxy{}
in Mint.HTTP1
to handle that case. But I want to hear what @whatyouhide thinks.
Under the hood UnsafeProxy
is using Mint.HTTP1
but returning Mint.HTTP1
from this function strikes me as leaking an implementation detail. I don't have enough context on what UnsafeProxy
is for or does, but from reading the code, it seems like Mint.HTTP1
but with some connection proxy details
I would just go with :http1
/:http2
. We can call the function major_protocol
or something like this so that we keep protocol/1
open for the future. Or even better, we have protocol(Conn.t()) :: :http1 | :http2
and in the future we can add protocol(Conn.t(), :with_minor_version) :: {...}
if the need arises.
@ericmj @whatyouhide how does that look to you?
Looks good but I think we should handle UnsafeProxy or did we decide not to?
@ericmj since Mint.HTTP.t()
is defined as Mint.HTTP1.t() | Mint.HTTP2.t()
I decided against it, but I can add it.
@whatyouhide it looks like using since: "1.4.0"
breaks on older elixir/erlang versions
@andyleclair you can do @doc since
conditionally, something like this:
if Version.compare(System.version(), "<whatever version introduced @doc since>") in [:eq, :gt] do
@doc since: "1.4.0"
end
@ericmj it seems like that could also be achieved by making
Mint.HTTP.conn_module
public, but I'm not sure aboutUnsafeProxy
. Since it usesMint.HTTP1
under the hood should we returnMint.HTTP1?
UnsafeProxy
? I don't really know whatUnsafeProxy
gets used for, guessing non-https proxying