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

Flag ZClient.request as deprecated #3133

Open
gaeljw opened this issue Sep 12, 2024 · 5 comments · May be fixed by #3154
Open

Flag ZClient.request as deprecated #3133

gaeljw opened this issue Sep 12, 2024 · 5 comments · May be fixed by #3154
Labels
💎 Bounty enhancement New feature or request

Comments

@gaeljw
Copy link

gaeljw commented Sep 12, 2024

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:

@deprecated("Use `batched` or `streaming` instead", since = "3.0.0")
def request(request: Request)(implicit trace: Trace): ZIO[Client & Scope, Throwable, Response] =

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:

def request(request: Request)(implicit ev: Body <:< In, trace: Trace): ZIO[Env & ReqEnv, Err, Out] = {

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. 🤷

@gaeljw gaeljw added the enhancement New feature or request label Sep 12, 2024
@kyri-petrou
Copy link
Collaborator

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
Copy link
Author

gaeljw commented Sep 12, 2024

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
Copy link
Member

jdegoes commented Sep 18, 2024

/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).

Copy link

algora-pbc bot commented Sep 18, 2024

💎 $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 bountyShare on socials

Attempt Started (GMT+0) Solution
🟢 @varshith257 #3154

@varshith257 varshith257 linked a pull request Sep 19, 2024 that will close this issue
Copy link

algora-pbc bot commented Sep 19, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💎 Bounty enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants