SocketApp refactoring, ability to provide custom websocket config #2338
Conversation
vigoo
requested review from
amitksingh1490,
jdegoes and
adamgfraser
as code owners
|
|
||
| import zio._ | ||
|
|
||
| final case class SocketApp[-R]( |
There was a problem hiding this comment.
maybe we can rename this WebSocketApp while we're at it.
|
|
||
| final case class SocketApp[-R]( | ||
| handler: Handler[R, Throwable, WebSocketChannel, Any], | ||
| customConfig: Handler[R, Throwable, Request, Option[WebSocketConfig]], |
There was a problem hiding this comment.
This seems too powerful. I'd prefer Option[WebSocketConfig], or if that cannot be done, something like <subset of Request> => WebSocketConfig.
But it feels like the config should be done at the route pattern level, e.g.:
val route = Method.GET / "connect" -> handler(socketApp.withConfig(...))You already know the route and pattern there and can set config on a per-route basis. You indeed have access to the full request.
There was a problem hiding this comment.
The author of the issue wanted to have a way to customize the handshaking based on the request (actual contents of some headers)
There was a problem hiding this comment.
<subset of Request> => WebSocketConfig would be great, especially if the result was passed into Handler.webSocket. As it is right now there's no way to negotiate the websocket sub-protocol for a single endpoint (such as choosing between graphql-ws and graphql-transport-ws), which would be really nice to do.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2338 +/- ##
==========================================
- Coverage 63.21% 62.82% -0.39%
==========================================
Files 135 136 +1
Lines 7032 7236 +204
Branches 1185 1230 +45
==========================================
+ Hits 4445 4546 +101
- Misses 2587 2690 +103
☔ View full report in Codecov by Sentry. |
|
Yes this should be in the next release |
Refactors
SocketApp[R]so it also implements aRequest => Option[WebSocketConfig]function that allows customizing the websocket connection including the handshake based on the actual request.Resolves #2278
/claim #2278