Skip to content
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

Introduce a synchronous web socket interface #1913

Merged
merged 4 commits into from
Aug 18, 2023
Merged

Introduce a synchronous web socket interface #1913

merged 4 commits into from
Aug 18, 2023

Conversation

adamw
Copy link
Member

@adamw adamw commented Jul 27, 2023

Since thanks to @benzwreck we now have websocket support in the default backend, below is a proposition to improve the "user experience" when using synchronous web sockets.

Of course, synchronous web sockets can be used with the existing API, but the user than has to deal with the Identity pseudo-monad (type alias), which wraps all results. This is visible in the WebSocketSynchronous example, where the request looks as follows (notice the type parameters, which are necessary for the example to compile):

def useWebSocket(ws: WebSocket[Identity]): Unit = ...

basicRequest
  .get(uri"wss://ws.postman-echo.com/raw")
  // alternatively, useWebSocket can be declared to return Identity[Unit]
  .response(asWebSocket[Identity, Unit](useWebSocket))

This can be improved by extracting the web socket API into separate traits, that is extracting from SttpApi the following three traits: SttpWebSocketAsyncApi (what we have today), SttpWebSocketSyncApi and SttpWebSocketStreamApi.

The sync version is a specialized variant of the async one, where each F[_] is ommited and internally replaced with Identity. Additionally, we can introduce a SyncWebSocket wrapper for WebSocket, which has type signatures without the F/Identity wrapper, i.e.:

class SyncWebSocket(val delegate: WebSocket[Identity]) {
  def receive(): WebSocketFrame = delegate.receive()
  def send(f: WebSocketFrame, isContinuation: Boolean = false): Unit = delegate.send(f, isContinuation)

This might be clearer when discovering the API. Summarizing the upsides:

(+) straightforward types when using web sockets in typical scenarios
(+) no F[_] in the synchronous API

But there are downsides as well:

(-) instead of the single import sttp.client4._, which brings in all the response specifications, users now how to also import the web socket api, i.e. import sttp.client4.ws.sync._ or import sttp.client4.ws.async._
(-) import suggestions for asWebSocket now yield two variants (sync and async)
(-) the Identity type alias still leaks in some places, e.g. in the types of response specifications returned by the api: def asWebSocket[T](f: SyncWebSocket => T): WebSocketResponseAs[Identity, Either[String, T]]

What do you think?

cc @szymon-rd, @bishabosha, @adpi2 (I think you wanted to do sth similar in the original sttp4 refactoring?)

Example code using the API (same as in examples):

import sttp.client4._
import sttp.client4.ws.SyncWebSocket
import sttp.client4.ws.sync._

object WebSocketSynchronous extends App {
  def useWebSocket(ws: SyncWebSocket): Unit = {
    def send(i: Int): Unit = ws.sendText(s"Hello $i!")
    def receive(): Unit = {
      val t = ws.receiveText()
      println(s"RECEIVED: $t")
    }
    send(1)
    send(2)
    receive()
    receive()
  }

  val backend = DefaultSyncBackend()

  try
    basicRequest
      .get(uri"wss://ws.postman-echo.com/raw")
      .response(asWebSocket(useWebSocket))
      .send(backend)
  finally backend.close()
}

Copy link
Contributor

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the following suggestion, I totally support this change. The tests and examples look good.

*
* See the `either` and `eitherClose` method to lift web socket closed events to the value level.
*/
class SyncWebSocket(val delegate: WebSocket[Identity]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move it to the root client4 package?

The idea is that everything should be available from a single import:

import sttp.client4._

object WebSocketSynchronous extends App {
  def useWebSocket(ws: SyncWebSocket): Unit = ???

  val backend = DefaultSyncBackend()

  try
    basicRequest
      .get(uri"wss://ws.postman-echo.com/raw")
      .response(ws.sync.asWebSocket(useWebSocket))
      .send(backend)
  finally backend.close()
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would rather go to the sttp-shared project, next to WebSocket: https://github.com/softwaremill/sttp-shared/blob/master/ws/src/main/scala/sttp/ws/WebSocket.scala

(it's shared among sttp client / tapir)

@adamw adamw merged commit 6f00ad2 into master Aug 18, 2023
14 checks passed
@mergify mergify bot deleted the sync-ws-api branch August 18, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants