rough draft wip for ws on AkkaHttp #196
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…is no apparent/intentional functional change Minor progress on ws solution for AkkaHttpAdapter
ghostdogpr
reviewed
Feb 3, 2020
| val apiRoute: Route = AkkaHttpAdapter.makeHttpServiceM(interpreter) | ||
| val wsApiRoute = { | ||
| // when do we create subscriptions? When do we make it effectful/unsafe? | ||
| // val subscriptions: UIO[Ref[Map[String, Fiber[Throwable, Unit]]]] = Ref.make(Map.empty[String, Fiber[Throwable, Unit]]) |
There was a problem hiding this comment.
subscriptions should be created inside the adapter, the end-user don't need to see it. A new Ref should be created for each WS connection.
ghostdogpr
reviewed
Feb 3, 2020
| } ~ path("graphiql") { | ||
| getFromResource("graphiql.html") | ||
| } | ||
|
|
||
| // convention is normally 8080. Keep at 8088? |
There was a problem hiding this comment.
Because so many services use 8080, I always end up with conflicts. I'd prefer to keep 8088 for this reason.
…is no apparent/intentional functional change Minor progress on ws solution for AkkaHttpAdapter
# Conflicts: # examples/src/main/scala/caliban/akkahttp/ExampleApp.scala
# Conflicts: # examples/src/main/scala/caliban/akkahttp/ExampleApp.scala
…eateFlow, createSource, route for ws/graphql, use handleMessagesWithSinkSource with subprotocol
…ketRoute respectively Added wrappers to Akka example interpreter removed silencer in sbt for AkkaHttp project onGraphQLResponse is now almost identical to http4s version processMessage is now almost identical to http4s version except shortcoming of stop handling, which needs more work removed prints pending issues: mutations: return only partial data unlike http4s lifecycle of subscriptions is unclear and could be quite wrong. For instance when routes are created multiple times from Caliban we see loads of duplicate answers, though that's not necessarily a susbscription issue. It's not perfectly clear when it should be recreated (done in createSink) definition and usage trait Protocol with Complete, Fail is suspect (see createSource usage of Source.actorRef comment below) processMessage, connection terminate case should close cleanly and it's not immediately obvious how to do it (possibly some via hook in createSink/Flow) usage of Actor to send messages back is weak in terms of back-pressure. Akka Http and Streams users recommend instead using some kind of queue. This needs investigation. createSource usage of Source.actorRef requires a CompletionMatcher and FailureMatcher and it's not clear how we should handle those.
|
Comments on where we stand here.
Pending issues:
|
|
subsumed by #219. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor progress on ws solution for AkkaHttpAdapter, stuck when using Altair
some potential proposal of refactoring of Http4sAdapter though there is no apparent/intentional functional change. I personally like it better, but it's not needed for sure for this PR.