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

[DRAFT] httpx async transport #340

Closed

Conversation

qw-in
Copy link

@qw-in qw-in commented Jul 7, 2022

Fix #154

Hello, not sure if there is still interest in supporting an httpx transport out of the box. I would be happy to maintain a separate package if that is preferable. Either way, thanks for having a look!

I've really just implemented the bare minimum so far:

  • Wiring up the optional dependencies
  • Copied over basic transport for aiohttp
  • Copied over the tests for aiohttp - skipping the ones not yet relevant

Would love some guidance on a few things:

  1. Is it worth supporting both httpx.Client and httpx.AsyncClient?
  2. I am weighing the trade-offs of adding extra configuration options (such as cookies) to the transport vs passing in an instance of AsyncClient. Leaning towards the second option. If a client is passed in, transport is not responsible for closing it. A usage example:
    transport = HTTPXAsyncTransport(url="...", cookies=httpx.Cookies(), **etc)
    # VS
    async with httpx.AsyncClient(cookies=httpx.Cookies(), **etc) as client:
        transport = HTTPXAsyncTransport(url="...", client=client)
  3. Is it worth trying to reuse the aiohttp tests for httpx rather than duplicate?

@leszekhanusz
Copy link
Collaborator

leszekhanusz commented Jul 7, 2022

Hello, not sure if there is still interest in supporting an httpx transport out of the box. I would be happy to maintain a separate package if that is preferable. Either way, thanks for having a look!

Yes, Thanks for looking into this!

Would love some guidance on a few things:

1. Is it worth supporting both `httpx.Client` and `httpx.AsyncClient`?

Maybe? But if we only support AsyncClient at first it's great too.

2. I am weighing the trade-offs of adding extra configuration options (such as `cookies`) to the transport vs passing in an instance of `AsyncClient`. Leaning towards the second option. If a client is passed in, transport is not responsible for closing it. A usage example:
   ```python
   transport = HTTPXAsyncTransport(url="...", cookies=httpx.Cookies(), **etc)
   # VS
   async with httpx.AsyncClient(cookies=httpx.Cookies(), **etc) as client:
       transport = HTTPXAsyncTransport(url="...", client=client)
   ```

No, only the first option is possible, the gql client should have the power to connect and close the connection itself. It is necessary for example for the permanent reconnecting session feature.

What you can (and should) do is to add a client_session_args argument like in the aiohttp transport which would allow the user to pass any desired extra arguments to httpx.

3. Is it worth trying to reuse the `aiohttp` tests for `httpx` rather than duplicate?

No, It's ok if the tests are copy-pasted to be simpler and closer to reality. DRY is not required for tests IMHO.

@leszekhanusz
Copy link
Collaborator

Some tips for the CI:

  • always run make check before a commit to ensure that the linting issues get corrected.
  • The first time, it is a good idea to run tox to ensure that everything works locally with all Python versions before pushing (See CONTRIBUTING.md)

@qw-in
Copy link
Author

qw-in commented Jul 13, 2022

I'll get back to this in the next couple weeks. Got a bit sidetracked with syntax highlighting over in graphiql.

Thanks for the clarification, glad I didn't get any further with implementation before asking

@jpavlav
Copy link

jpavlav commented Sep 13, 2022

@qw-in I'd love to help out and get this off and running (just making some adjustments so CI passes). I have a project that'd benefit from an httpx transport. Mind if I just push an/some update/s to your feature branch?

@qw-in
Copy link
Author

qw-in commented Sep 13, 2022

@jpavlav absolutely, feel free to take over the branch!

Not sure if you'll be able to directly since it's a fork but I'll happily close this pr in favour of yours. Thanks!

@helderco helderco mentioned this pull request Nov 15, 2022
4 tasks
@helderco helderco mentioned this pull request Nov 26, 2022
@helderco
Copy link
Contributor

Hi, I need this for work and found it easier to contribute an alternative PR:

@qw-in qw-in closed this Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPX Transport
4 participants