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

Port async-http-client to ce3 #4149

Merged
merged 8 commits into from Jan 8, 2021
Merged

Port async-http-client to ce3 #4149

merged 8 commits into from Jan 8, 2021

Conversation

RaasAhsan
Copy link
Member

No description provided.

()
F.delay(
httpClient.executeRequest(toAsyncRequest(req, dispatcher), asyncHandler(cb, dispatcher)))
.as(None)
})
}

/** Allocates a Client and its shutdown mechanism for freeing resources.
*/
def allocate[F[_]](config: AsyncHttpClientConfig = defaultConfig)(implicit
Copy link
Member Author

@RaasAhsan RaasAhsan Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need allocate and stream constructors? I have similar sentiments on apply. At this point they should probably stay for compatibility, but I think having a single resource constructor is solid (I suspect most users are using the resource constructor anyways)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why we did. I was going to say to DRY up with .stream, but even that uses .resource. I'd be happy to deprecate the superfluous ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply takes one that's already configured, and thus can be an implementation other than DefaultAsyncHttpClient and is to my eyes more justifiable than allocate and stream. It does feel less like an apply after getting the dispatcher argument, however. And not providing it means lifting the result into a Resource that doesn't actually shut down the client. I think you've done the best you can do here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also change the apply signature to something like:

def apply[F[_]](httpClient: AsyncHttpClient)(implicit F: Async[F]): Resource[F, Client[F]]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource didn't have allocate when we initally created these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love still calling it apply when it doesn't return the companion class (or when the return type changes!), but I prefer this signature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth a rename to fromClient, will do that

@rossabaker rossabaker added this to To do in Cats Effect 3 via automation Jan 7, 2021
@rossabaker rossabaker closed this Jan 8, 2021
@rossabaker rossabaker deleted the branch http4s:main January 8, 2021 18:48
Cats Effect 3 automation moved this from To do to Done Jan 8, 2021
@rossabaker
Copy link
Member

Incorrectly autoclosed when cats-effect-3 branch was merged.

@rossabaker rossabaker reopened this Jan 8, 2021
@rossabaker rossabaker changed the base branch from cats-effect-3 to main January 8, 2021 18:54
@rossabaker rossabaker merged commit aa33ce2 into http4s:main Jan 8, 2021
armanbilge pushed a commit to http4s/http4s-async-http-client that referenced this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants