-
-
Notifications
You must be signed in to change notification settings - Fork 10
Feature/continue to http #111
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
base: main
Are you sure you want to change the base?
Feature/continue to http #111
Conversation
What does continueToHTTP do that dontUpgrade doesn't already support? EDIT: is this for returning a custom HTTP response on a failed upgrade? |
I don't know a great deal about socket.io, but it looks like it goes through a non-standard way to upgrade to websocket. ie it doesn't send the standard upgrade request. The spec sample session doesn't detail any headers sent or if any are sent at all https://socket.io/docs/v4/socket-io-protocol/#sample-session
So none of the Hummingbird WebSocket code will be triggered as the lower level SwiftNIO isn't triggered. |
The naming of the enum might be confusing, sorry about that. This PR enables support for the Socket.IO handshake protocol. After this handshake, the connection upgrades to use WebSockets. With the existing implementation, I couldn't find a way to handle the initial handshake on the application layer. My assumption was that, once the handshake and upgrade were complete, all further communication would happen over standard WebSockets. Are you saying that—even after a successful handshake and upgrade—communication still wouldn’t work because Socket.IO doesn’t fully adhere to the WebSocket protocol? |
Does a socket.io websocket upgrade request include the headers I posted previously? As far as I can tell from the spec sample session it doesn't. And your code won't work, as it is doesn't have the required headers to instigate an HTTP upgrade |
case upgrade(HTTPFields = [:]) | ||
case continueToHTTP | ||
case httpResponse(Response) | ||
} |
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.
Adding enum cases is a breaking change. Unfortunately I made this a public enum, instead of a struct. I've wanted to do something similar to .httpResponse
before but come up against this.
I'm opening this draft PR because I ran into a case that I believe isn't currently covered required for socket.io. I have added a
continueToHTTP
case toShouldUpgradeResult
.The (mostly vibe coded) code is still rough and needs cleanup, but before spending more time on it, I wanted to see if there’s interest in supporting this use case.
If this is a direction the project wants to go in, I’ll tidy up the code and update the branch.