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

Fix httpx private api usage #2534

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

karpetrosyan
Copy link
Member

Refs: encode/httpx#3130

In some places, starlette uses imports from the private API, such as httpx._client, which will not work after mere of encode/httpx#3130.

@karpetrosyan karpetrosyan mentioned this pull request Mar 3, 2024
@Kludex
Copy link
Sponsor Member

Kludex commented Mar 3, 2024

Actually, we use future annotations here. Nothing will break at runtime. 🥹🙏

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 3, 2024

Is there any discussion about making types available on public modules?

@karpetrosyan
Copy link
Member Author

Is there any discussion about making types available on public modules?

USE_CLIENT_DEFAULT was already public, we can use it directly from httpx.

@tomchristie
Copy link
Member

I'd suggest including the type definitions explicitly inside Starlette's codebase.

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 5, 2024

I'd suggest including the type definitions explicitly inside Starlette's codebase.

Ok. 👍

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 5, 2024

I'd suggest including the type definitions explicitly inside Starlette's codebase.

Ok. 👍

Wanna do it here @karpetrosyan ?

@karpetrosyan
Copy link
Member Author

Wanna do it here @karpetrosyan ?

Yep

@karpetrosyan
Copy link
Member Author

Do we really can add them into the starlette?
There are types that should be passed through the httpx, how we should handle them?

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 9, 2024

For UseClientDefault we can't. It's a class, and is private. cc @tomchristie

@tomchristie
Copy link
Member

Noted. I guess there's type(httpx.USE_CLIENT_DEFAULT)

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 9, 2024

Noted. I guess there's type(httpx.USE_CLIENT_DEFAULT)

We can't use that for type annotation. 👀

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 20, 2024

What's the plan here? @karpetrosyan

@karpetrosyan
Copy link
Member Author

What's the plan here? @karpetrosyan

I believe the best we can do here is to limit the upper version of HTTPX to the current version. Alternatively, we can maintain this import in HTTPX only for Starlette, but I don't believe this is a good idea. Is it critical that older versions of Starlette do not work with the most recent HTTPX?

@tomchristie
Copy link
Member

Shall we raise an issue for "3rd party packages using private imports of httpx"? That issue should describe what private imports starlette is currently using and why, so we can resolve whatever is causing that.

@tomchristie
Copy link
Member

(I say "3rd party" here because we shouldn't treat other encode packages differently from the rest of the ecosystem. Starlette's use of some private importing will be a really helpful thing for us to work through what we're getting wrong. I expect we need to think a bit more carefully about our typing and API expectations, let's see)

@karpetrosyan
Copy link
Member Author

I have raised encode/httpx#3176 to track all kinds of API needs in third-party packages.

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.

None yet

3 participants