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

Use HTTPX exclusively, sender changes #139

Closed
felix-hilden opened this issue Feb 2, 2020 · 8 comments · Fixed by #201
Closed

Use HTTPX exclusively, sender changes #139

felix-hilden opened this issue Feb 2, 2020 · 8 comments · Fixed by #201
Assignees
Labels
change Change behaviour of an existing component
Milestone

Comments

@felix-hilden
Copy link
Owner

From #131. When Httpx is stable, we could drop Requests and use it exclusively for senders. Currently it is used for async, which is beyond Requests' capability. There would be less hassle with converting between the two libraries.

@felix-hilden felix-hilden added waiting Waiting for another event change Change behaviour of an existing component labels Feb 2, 2020
@felix-hilden felix-hilden added this to the v2.0.0 milestone Feb 2, 2020
@felix-hilden felix-hilden self-assigned this Feb 2, 2020
@felix-hilden felix-hilden mentioned this issue Feb 2, 2020
8 tasks
@felix-hilden felix-hilden mentioned this issue May 7, 2020
11 tasks
@felix-hilden
Copy link
Owner Author

HTTPX 1.0 has been pushed to "mid-2020", and they are now working on 0.13. We might want to release our 2.0 sooner. Our 3.0 will likely be less breaking than 2.0, so things should go a bit smoother that time around.

Alternatively, we could simply adopt the current version. I'm not sure how mature they are, but our use case is pretty simple. I'll investigate a bit. At least 0.12 broke nothing for us.

@felix-hilden
Copy link
Owner Author

We'll stay with Requests and HTTPX for now.

@felix-hilden felix-hilden modified the milestones: v2.0.0, v3.0.0 May 21, 2020
@felix-hilden
Copy link
Owner Author

Looks like a 0.14 will be released before 1.0 as well. I think our 3.0 will wait for HTTPX to be stable. Surely it will come soon enough and the only other item on the changelist is PKCE (#193).

@felix-hilden
Copy link
Owner Author

felix-hilden commented Aug 9, 2020

We should also really consider whether all the different types of senders are necessary. There's something to be said about not even offering transient and singleton senders. I can not conjure an example where transient senders would be useful beyond debugging. Similarly, having multiple instances of Spotify should be rare, as we already offer options for swapping tokens. And those rare cases can definitely deal with using default sender instances or passing clients around.

Having senders is fine in my opinion, rather than using e.g. caching and retry arguments in the client directly, because that allows users to build their own. But this idea could be even expanded. Currently we pass requests.Request to senders. If we were to pass request parameters, users potentially even use different libraries in place of HTTPX without translating between httpx.Request and whatever they are using, like we have done so far. And indeed we cannot pass a request around, because then client options are not merged with the request as it is being sent (encode/httpx#1152).

@felix-hilden felix-hilden changed the title Use Httpx exclusively Use HTTPX exclusively, change sender API Aug 10, 2020
@felix-hilden
Copy link
Owner Author

So, given the absence of requests.Session.prepare_request() and the fact that pre-built HTTPX requests are sent as-is, we have three options.

  • Change interface from Request -> Response to method, url, headers, ... -> Response, or even -> code, headers, json or such, allowing for wildly custom senders. This complicates caching a bit.
  • Expose httpx.Client.build_request through senders for our clients to build requests with.
  • Implement our own light request and response containers. They would need:

Request

  • method
  • url
  • params
  • headers
  • data

Response

  • url (with params)
  • headers
  • status_code
  • json method (that maybe returns None when no content)

Honestly, I'm not sure, but I think exposing build_request through senders is my least favorite. I'm flipping between the simpler interface that using a custom request-response would provide versus the implementation simplicity of a list of values.

@felix-hilden
Copy link
Owner Author

I'm leaning towards using our own request and response wrappers, for simplicity.

We could also get rid of sender options. Now they are a bit unintuitive. Again, having multiple senders instantiated is not really expected. And with the adoption of HTTPX, its Client can be passed with all options already set, with no need of passing parameters to each request. Removing all options would be a welcome simplification.

@felix-hilden felix-hilden changed the title Use HTTPX exclusively, change sender API Use HTTPX exclusively, sender changes Aug 15, 2020
@felix-hilden
Copy link
Owner Author

Here's a summary of the proposed changes:

  • Drop Requests, use HTTPX only
  • Define custom Request and Response wrappers to be used in the sender interface
  • Remove all sender options
  • Remove transient and singleton senders, rename persistent senders as the former bases SyncSender and AsyncSender
  • Inherit web exceptions from builtin Exception rather than Requests' or HTTPX's top exception
  • Clients inherit from ExtendingSender
  • CachingSender arguments are in the same order as RetryingSender

This was referenced Aug 15, 2020
@felix-hilden
Copy link
Owner Author

Upgrading the HTTPX dependency to 1.0 is the only thing left to be done now. It's a check in #202.

@felix-hilden felix-hilden removed the waiting Waiting for another event label Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Change behaviour of an existing component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant