-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix dx serve proxying of websocket connections established from the browser #3895
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 dx serve proxying of websocket connections established from the browser #3895
Conversation
ad90a06
to
9c8930e
Compare
@ealmloff Hi, just wondering if there's anything I need to/can do here to help move this along? If you're not interested in the change I'm happy to close the PR, just let me know whichever way. |
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.
@ealmloff Hi, just wondering if there's anything I need to/can do here to help move this along? If you're not interested in the change I'm happy to close the PR, just let me know whichever way.
Thanks for the ping. I think there were some conflicts with #3820. If you can fix those issues, this looks good to me once checks pass. I confirmed this works with axum websockets and with the server function websocket integration.
Requesting Jon's review on this as well since he wrote the original websocket proxy logic
I fixed the compilation errors following the Axum upgrade. Unfortunately even though Tungstenite and Axum now use the same Not that I expect it'll actually have a measurable impact on performance, it just feels a bit silly. |
It's not entirely clear to me what the cause of the Playwright issues is, but I don't think it's related to these changes? From what I can tell digging through the logs it looks like the |
The playwright test failures are not related. Playwright is failing on main. #4042 fixes some of the issues |
This PR makes the following changes:
Upgrade
header to detect websocket connections where the URI doesn't include the schemeThe explicit proxying is required as browsers (at least Firefox and Chrome) don't handle redirect responses. E.g. Chrome emits an error complaining about an invalid response.
This PR is somewhat of a proof of concept in its current state. I'm happy to make any necessary changes to conform to Dioxus's code style and general guidelines, and to discuss alternative approaches.
Fixes #3894