-
Notifications
You must be signed in to change notification settings - Fork 422
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
Added ZIO based server for netty #2574
Conversation
build.sbt
Outdated
.settings( | ||
name := "tapir-netty-server-zio", | ||
libraryDependencies ++= Seq( | ||
"io.netty" % "netty-all" % "4.1.82.Final", |
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.
let's extract netty's version to a constant in the build so that it's not repeated
build.sbt
Outdated
"dev.zio" %% "zio-interop-cats" % Versions.zioInteropCats, | ||
) ++ loggerDependencies, | ||
// needed because of https://github.com/coursier/coursier/issues/2016 | ||
useCoursier := false |
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.
maybe some common netty-settings to avoid duplication?
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.
I think that not much things can be move to common as most setting is different between backends
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.
even if it's 2 settings let's factor them out so that we don't repeat the comments etc.
case class NettyZioServer[R, SA <: SocketAddress](routes: Vector[Route[RIO[R, *]]], options: NettyZioServerOptions[R, SA])(implicit | ||
runtime: Runtime[R] | ||
) { | ||
def addEndpoint(se: ServerEndpoint[Any, RIO[R, *]]): NettyZioServer[R, SA] = addEndpoints(List(se)) |
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.
I think here we should use ZServerEndpoint
from sttp.tapir.ztapir
import sttp.tapir.server.netty.internal.FutureUtil | ||
import zio.{RIO, ZIO} | ||
|
||
object ZioUtil { |
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.
this can be private?
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.
and this function is available from the parent project?
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.
I am not sure which one ? FutureUtil is public
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.
Ah it's a conversion to RIO, ok. Does it need to be public? It looks like an impl detail
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.
already moved to private in zio
Looks good - docs are missing :) |
Seventh task in #499 |
You'll also probably need to implement this: #2579 in the ZIO variant ( |
closes in favor of #2602 |
No description provided.