apicollective/apibuilder-generator

http4s client 0_18: Replace PooledHttp1Client (deprecated) with Http1Client

Closed this issue · 3 comments

I noticed that the client is using the deprecated PooledHttp1Client.

I think it would be nice to replace it with Http1Client, which also provides a nice stream method that brackets the client so that it's safely automatically closed at the end of the stream (this would probably involve changing some functions to return streams, I think that's way better, but this is a breaking change).

There are other issues with the current code. The Client constructor creates a client if one is not provided - this is a side-effect, so Client creation must be wrapped in an effect type.

In the Client companion object, the defaultAsyncHttpClient internal singleton state is not in line with best practices in the http4s world (which avoids such state where possible).

Recommended:

  1. Remove the default value for the asyncHttpClient parameter.
  2. There is no great way to create a singleton value for the client - it is probably not worth it. Users should do the following:
  override def stream(args: List[String], requestShutdown: F[Unit]): Stream[F, StreamApp.ExitCode] = {
  for {
    httpClient <- Http1Client.stream[F]
    apiClient = MyApiClient(..., asyncHttpClient = httpClient)
    exitCode <- BlazeBuilder[F].(...).serve
  } yield exitCode
}
  1. The asyncHttpClient parameter is misnamed in this world where that library may not be used, this should just be httpClient or something like that.

I agree with this, I can do it as part of #336

The client changes are done, can you review the client too? :) I need an extra pair of eyes on it