vapor/vapor

WebSocket routing is very limited...

Closed this issue · 3 comments

Vapor currently has a single route builder for websockets:

@discardableResult
public func webSocket(
    _ path: PathComponent...,
    maxFrameSize: WebSocketMaxFrameSize = .`default`,
    onUpgrade: @escaping (Request, WebSocket) -> ()
) -> Route {
    return self.on(.GET, path) { request -> Response in
        let res = Response(status: .switchingProtocols)
        res.upgrader = .webSocket(maxFrameSize: maxFrameSize, onUpgrade: { ws in
            onUpgrade(request, ws)
        })
        return res
    }
}

This route has several limitations:

  1. The upgrade closure isn't marked throws, so integrating throwing code is much more cumbersome than other routes.
    • What would automatic error propagation for websockets look like?
  2. There is no additional overload that takes an array instead of a variadic path, so it's not possible to provide your own websocket routes that call the base route.
  3. websocket routes can't be provided by consumers because Request.upgrader and the associated Upgrader type are internal.

I'd like to fix these limitations, but there are several approaches to take:

  1. Just make Request.upgrader and Upgrader public so users can write their own routes which upgrade to a websocket connection.
  2. Just add additional overloads so users can make their own routes, but are limited to calling the base route.
  3. Add full error handling so the need for custom routes is limited. However, I'm not sure what that handling should be, as websocket handling is somewhat different than normal requests.

I can do 1 and/or 2 myself, but would need more guidance on 3. I just need to know which approach is preferred.

Hi @jshier, thanks for the feedback.

  1. The upgrade closure isn't marked throws, so integrating throwing code is much more cumbersome than other routes.
    What would automatic error propagation for websockets look like?

This is a tough one. Once the WebSocket connection is established, you should inform the connected client of any errors. Users of this API will already have their own data structures for doing this and it would be annoying if Vapor sent arbitrary JSON that you couldn't control.

I think a higher level API that takes more control over the structure of the packets would be a better place to put something like this.

  1. There is no additional overload that takes an array instead of a variadic path, so it's not possible to provide your own websocket routes that call the base route.

This is an issue. Related to #2420. As I wrote there, all public variadic methods in vapor should have an array-accepting counterpart.

  1. websocket routes can't be provided by consumers because Request.upgrader and the associated Upgrader type are internal.

This is a duplicate of #2235


A PR for (2) would be great :) If not, please open a separate issue for that and I will tackle it.
As for (1), that seems outside the scope of Vapor at least for now. Though a separate issue / Swift forum post discussing this more could be good.

Closing this issue.

This is a tough one. Once the WebSocket connection is established, you should inform the connected client of any errors. Users of this API will already have their own data structures for doing this and it would be annoying if Vapor sent arbitrary JSON that you couldn't control.

I think a higher level API that takes more control over the structure of the packets would be a better place to put something like this.

How is this different than the normal routes that return a default payload on error? And what does this higher level API look like to you?

A PR for (2) would be great :)

Sure, I'll take a look when I get a chance, probably this weekend.

How is this different than the normal routes that return a default payload on error? And what does this higher level API look like to you?

It's a similar problem to normal routes. Vapor has things like Abort, ErrorMiddleware, and req.content for handling content. WebSockets could have something similar. I haven't really thought much about how that would look for websockets yet. I'd probably want to research how other web frameworks do it first.

Sure, I'll take a look when I get a chance, probably this weekend.

Thanks!