-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
Integrate websockets into the async base view #3638
Integrate websockets into the async base view #3638
Conversation
Reviewer's Guide by SourceryThis pull request integrates WebSocket functionality into the async base view, resulting in more consistent WebSocket behavior across different integrations. The changes involve refactoring the WebSocket handling logic, removing redundant code, and updating various integrations to use the new unified approach. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @DoctorJohn - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more comprehensive error handling for WebSocket connections, especially for edge cases that might occur during runtime.
- It would be beneficial to include performance benchmarks to ensure that the centralization of WebSocket logic doesn't introduce any significant overhead.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Thanks for adding the Here's a preview of the changelog: Starting with this release, WebSocket logic now lives in the base class shared between all HTTP integrations. Here's the tweet text:
|
Thanks for adding the Here's a preview of the changelog: Starting with this release, WebSocket logic now lives in the base class shared between all HTTP integrations. Here's the tweet text:
|
2d01e81
to
1430fe5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3638 +/- ##
==========================================
+ Coverage 96.76% 96.82% +0.05%
==========================================
Files 522 503 -19
Lines 33824 33409 -415
Branches 5635 5583 -52
==========================================
- Hits 32731 32348 -383
+ Misses 863 830 -33
- Partials 230 231 +1 |
CodSpeed Performance ReportMerging #3638 will not alter performanceComparing Summary
|
d46fbbb
to
abac0e4
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
This PR makes me so happy <3 |
@patrick91 wait until we get the incremental delivery going 🫢 [different pr of course] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid!
Description
This PR introduces adapter classes for WebSockets, similar to the adapter classes for HTTP requests. This allows us to share all common WebSocket logic between all integrations.
As a result, WebSockets now behave much more consistently between different integrations, which allows us to share all WebSocket tests between them.
I put much energy into keeping this a non-breaking change and keeping it as reviewable as possible. As a result, the Channels integration still looks as funny as before, i.e., its HTTP and WS handlers are still split. This deserves another look in a separate PR.
Types of Changes
Summary by Sourcery
Integrate WebSocket adapter classes into the async base view to unify WebSocket logic across integrations, ensuring consistent behavior and simplifying maintenance. Remove redundant WebSocket handler implementations and update tests to reflect the new architecture.
New Features:
Enhancements:
Tests:
Chores: