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

Revamp implementation without BaseHTTPMiddleware #41

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

frankie567
Copy link
Contributor

Hello there 👋

Because of the numerous limitations of the BaseHTTPMiddleware class provided by Starlette, the Starlette dev' team is about to deprecate it and encourage people to write "pure" ASGI middlewares. In particular, one of this limitation causes the issue #33 here.

This PR is an attempt at converting the existing middleware without BaseHTTPMiddleware. The resulting code is quite similar, the only "tricky" thing is the part where we wrap the send function with our own; as it's the common way to do in ASGI.

All existing tests are passing.

I would be glad to discuss it and make all changes needed so we can integrate this new approach in the library.

Cheers!

@Kludex
Copy link

Kludex commented Jul 22, 2022

The deprecation is not decided yet... But the motivation for changing the implementation is valid. 👍

@adriangb
Copy link

The deprecation has now been decided. I would recommend moving forward with this.

@PyDataBlog
Copy link

Hello there 👋

Because of the numerous limitations of the BaseHTTPMiddleware class provided by Starlette, the Starlette dev' team is about to deprecate it and encourage people to write "pure" ASGI middlewares. In particular, one of this limitation causes the issue #33 here.

This PR is an attempt at converting the existing middleware without BaseHTTPMiddleware. The resulting code is quite similar, the only "tricky" thing is the part where we wrap the send function with our own; as it's the common way to do in ASGI.

All existing tests are passing.

I would be glad to discuss it and make all changes needed so we can integrate this new approach in the library.

Cheers!

Looks like this project is no longer maintained. Interested in forking and updating the package? @frankie567

@frankie567
Copy link
Contributor Author

Hello there 👋

Because of the numerous limitations of the BaseHTTPMiddleware class provided by Starlette, the Starlette dev' team is about to deprecate it and encourage people to write "pure" ASGI middlewares. In particular, one of this limitation causes the issue #33 here.

This PR is an attempt at converting the existing middleware without BaseHTTPMiddleware. The resulting code is quite similar, the only "tricky" thing is the part where we wrap the send function with our own; as it's the common way to do in ASGI.

All existing tests are passing.

I would be glad to discuss it and make all changes needed so we can integrate this new approach in the library.

Cheers!

Looks like this project is no longer maintained. Interested in forking and updating the package? @frankie567

I appreciate your trust, but I'm already involved in several packages and projects, not sure I can commit to support another one ☺️

@PyDataBlog
Copy link

Hello there 👋
Because of the numerous limitations of the BaseHTTPMiddleware class provided by Starlette, the Starlette dev' team is about to deprecate it and encourage people to write "pure" ASGI middlewares. In particular, one of this limitation causes the issue #33 here.
This PR is an attempt at converting the existing middleware without BaseHTTPMiddleware. The resulting code is quite similar, the only "tricky" thing is the part where we wrap the send function with our own; as it's the common way to do in ASGI.
All existing tests are passing.
I would be glad to discuss it and make all changes needed so we can integrate this new approach in the library.
Cheers!

Looks like this project is no longer maintained. Interested in forking and updating the package? @frankie567

I appreciate your trust, but I'm already involved in several packages and projects, not sure I can commit to support another one ☺️

Fair enough. Ok, with you if your suggestions is used as the base for the new project?

@frankie567
Copy link
Contributor Author

Of course, that's open source!

@perdy perdy merged commit 34aa2f7 into perdy:master Feb 5, 2024
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.

5 participants