zio / zio-http

A next-generation Scala framework for building scalable, correct, and efficient HTTP clients and servers
https://zio.dev/zio-http
Apache License 2.0
796 stars 400 forks source link

Flag ZClient.request as deprecated #3133

Open gaeljw opened 1 month ago

gaeljw commented 1 month ago

I've seen that with 3.0.0-RC10 (and 3.0.0), the ZClient was refactored regarding the need for Scope which is great.

However I noticed that the ZClient.request method is deprecated in favour of batched and streaming only in the companion object: https://github.com/zio/zio-http/blob/8c70090aa01442c3e09193e468917adfa0aa2286/zio-http/shared/src/main/scala/zio/http/ZClient.scala#L296-L297

If you already have an instance of ZClient, the method request is still accessible and it's not obvious that it's oriented for streaming usage: https://github.com/zio/zio-http/blob/8c70090aa01442c3e09193e468917adfa0aa2286/zio-http/shared/src/main/scala/zio/http/ZClient.scala#L178

As a user, I would've expected to see that the request method (of the class) should not be used directly.

I guess there's not that much we can do now without breaking binary compatibility though. :shrug:

kyri-petrou commented 1 month ago

Just my 2 cents: The issue with the request method companion object (and the reason it was deprecated / removed) is that we wanted users to re-evaluate whether they needed the streaming capability or not.

w.r.t the request method on the Client class, it exists both for the "streaming" client and the batched client. If used in a streaming client (i.e., ReqEnv =:= Scope) then it's streaming, otherwise it's not. We can potentially deprecate it for consistency if more users feel the same way towards it

gaeljw commented 1 month ago

To give more context, the thing is (and maybe I'm not like most people) I don't use the companion object at all. I have a service/class that gets "injected" a ZClient (well the alias Client actually, through ZLayers..) and is thus calling client.request(...). If I hadn't read the changelog, I would not have noticed I could get rid of the Scope because I'm just doing simple requests without streaming.

For sure, we'd need more feedback on the matter to make a choice :)

jdegoes commented 1 month ago

/bounty $75 for deprecating ZClient#request (the method on the trait), and for introducing a new def streaming method, which would be the analogue to the def batched method, and which would be the mirror of ZClient.streaming, requiring the environment additions include Scope.

This way, anyone using an instance of ZClient will now receive a warning and they can decide whether to use batched or streaming. Same as for those using the accessors (which I don't recommend -- I prefer @gaeljw's method of accessing client, personally).

algora-pbc[bot] commented 1 month ago

💎 $75 bounty • ZIO

Steps to solve:

  1. Start working: Comment /attempt #3133 with your implementation plan
  2. Submit work: Create a pull request including /claim #3133 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to zio/zio-http!

Add a bounty • Share on socials

Attempt Started (GMT+0) Solution
🟢 @varshith257 #3154
algora-pbc[bot] commented 1 month ago

💡 @varshith257 submitted a pull request that claims the bounty. You can visit your bounty board to reward.