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

Replace BaseHTTPMiddleware with pure ASGI middleware #173

Merged
merged 5 commits into from Jun 18, 2022

Conversation

adriangb
Copy link
Contributor

Hi, I am a Starlette dev. We are trying to discourage usage of BaseHTTPMiddleware.

@adriangb adriangb marked this pull request as ready for review June 13, 2022 14:35
@HassanAbouelela
Copy link
Member

Hello, thanks for your contribution. I'll try to get to it this week, but would you mind fixing the linting errors in the meantime? Thanks!

Copy link
Contributor Author

@adriangb adriangb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@HassanAbouelela
Copy link
Member

Hey I'm working on reviewing this right now, is there any reference for why we're moving away from this? I'm going through the documentation and I can't see anything on it or the recommended approach.

@HassanAbouelela
Copy link
Member

Alright, I see the issue you linked, I'll track there

@adriangb
Copy link
Contributor Author

Please check out the warnings in this section of the docs: https://www.starlette.io/middleware/#basehttpmiddleware

BaseHTTPMiddleware has bugs that we don't think can be fixed. We may deprecate it in the future (although that's not finalized), but as a first step we want to discourage its usage especially in libraries.

@HassanAbouelela
Copy link
Member

This project isn't a library, but we're happy to help where we can 👍.

backend/middleware.py Outdated Show resolved Hide resolved
backend/middleware.py Outdated Show resolved Hide resolved
adriangb and others added 2 commits June 17, 2022 17:28
Co-authored-by: Hassan Abouelela <abouelelahassan@gmail.com>
@HassanAbouelela
Copy link
Member

Everything looks fine, thanks for your contribution. Is there an ETA on when the docs will be updated? It's really difficult to deal with this as is.

@HassanAbouelela HassanAbouelela merged commit 7d4affd into python-discord:main Jun 18, 2022
@adriangb adriangb deleted the asgi-middleware branch June 18, 2022 00:45
@adriangb
Copy link
Contributor Author

What part of the docs are giving you trouble / would you like updated?

@HassanAbouelela
Copy link
Member

The largest issue right now is that they aren't merged, so no one will know where to look for them. This is only a temporary problem of course, but if you plan on making this same migration on projects where they need to edit the middleware more frequently, that can be a blocker.

More relevantly, there are a few minor things here and there. In no particular order:

  • The Reusing Starlette components section feels lacking. It explains how to go about creating a request and response object in the __call__, but does not explain how that will relate to the request and response within your actual codebase. For instance, I would never have gotten the trick you used for setting our request scope from what's written there.
  • This is related to the previous point, but the Per-request state confused me for a while. I assumed that's what I was looking for since it had all the right keywords, and I believed at first that the docs were saying we shouldn't do it anymore. Particularly, calling it request lead me to believe that it's about setting the scope on the request object, instead of the middleware class.

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

2 participants