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

cats-effect 3 and http4s 0.23.0 #891

Merged
merged 14 commits into from
Aug 1, 2021
Merged

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented May 19, 2021

This PR brings support of cats-effect 3.2.1 and http4s 0.23.

.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

I was waiting for a stable http4s release to start, good that you got ahead of me 😄
I need to dig into CE3, I imagine there is no way out of using Futures?

@iRevive
Copy link
Contributor Author

iRevive commented Jul 31, 2021

@ghostdogpr a stable version of http4s has been released. I will update the PR today

iRevive added 5 commits July 31, 2021 08:48
# Conflicts:
#	.circleci/config.yml
#	adapters/http4s/src/main/scala/caliban/Http4sAdapter.scala
#	build.sbt
#	examples/src/main/scala/example/federation/FederatedApp.scala
#	examples/src/main/scala/example/http4s/AuthExampleApp.scala
#	examples/src/main/scala/example/http4s/ExampleApp.scala
#	examples/src/main/scala/example/tapir/ExampleApp.scala
@iRevive iRevive changed the title WIP: CE 3 WIP: CE 3 and http4s 0.23.0 Jul 31, 2021
@iRevive
Copy link
Contributor Author

iRevive commented Jul 31, 2021

I need to dig into CE3, I imagine there is no way out of using Futures?

Yes, in CE3 futures the only way to materialize the effect. (apart from the .unsafeRunSync())

@ghostdogpr
Copy link
Owner

To not be blocked by finch and monix, we could let them depend on CE2 and upgrade only http4s and cats interop. I believe they are independent. Wdyt?

@iRevive
Copy link
Contributor Author

iRevive commented Jul 31, 2021

Sounds good to me. I will adjust the dependencies.

@iRevive
Copy link
Contributor Author

iRevive commented Jul 31, 2021

How should we deal with the examples project? We should keep either http4s + catsInterop or finch + monix.
Otherwise, we will have dependencies conflict.

As an option, I can make a separate project (e.g. examples-2) and move finch and monix examples there.

@ghostdogpr
Copy link
Owner

Ah, right, didn't think of that 😢
Considering monix and finch are much less used than cats-effect/http4s, maybe we just comment out the example code for those 2. So people who need it will still find the code, and we don't bother splitting the examples project. And we can uncomment when they are available.

Maybe just add a comment at the top of the file to explain why it's commented out.

@@ -327,7 +324,7 @@ object Http4sAdapter {
} yield response
}

def makeWebSocketService[R, E](
def makeWebSocketService[R <: Has[_] with Clock with Blocking, E](
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the compilation fails without <: Has[_] with Clock with Blocking in Scala 3.

But compiles fine in 2.13.6.

Copy link
Owner

Choose a reason for hiding this comment

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

Where does the constraint come from?

Copy link
Owner

Choose a reason for hiding this comment

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

I tried your branch and was able to make it work: change the queues to be of Task instead of RIO[R, *] and it will solve the problem. You also need to put .provideLayer(Clock.live) back.

There's still an error but I think it's something else.

Copy link
Owner

Choose a reason for hiding this comment

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

I think I made it work, will open a PR to your repo.

Copy link
Owner

Choose a reason for hiding this comment

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

I was able to push to your branch directly, you can have a look

@iRevive
Copy link
Contributor Author

iRevive commented Jul 31, 2021

I've updated the docs and the definition of the build.

@iRevive iRevive changed the title WIP: CE 3 and http4s 0.23.0 cats-effect 3 and http4s 0.23.0 Jul 31, 2021
@@ -233,24 +238,26 @@ lazy val http4s = project
.settings(name := "caliban-http4s")
.settings(commonSettings)
.settings(
crossScalaVersions -= scala3,
Copy link
Owner

Choose a reason for hiding this comment

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

It supports scala 3, nice!

@@ -327,7 +324,7 @@ object Http4sAdapter {
} yield response
}

def makeWebSocketService[R, E](
def makeWebSocketService[R <: Has[_] with Clock with Blocking, E](
Copy link
Owner

Choose a reason for hiding this comment

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

Where does the constraint come from?

@@ -366,14 +366,15 @@ object Http4sAdapter {
for {
msg <- RIO.fromEither(decode[Json](text))
msgType = msg.hcursor.downField("type").success.flatMap(_.value.asString).getOrElse("")
_ <- RIO.whenCase(msgType) {
_ <- RIO.whenCase[R, String](msgType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it didn't compile in Scala 3 because explicit types were missing.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah weirdly this one seems necessary with Scala 3.

@iRevive
Copy link
Contributor Author

iRevive commented Jul 31, 2021

@ghostdogpr thank you for the improvements. Looks much better now

@ghostdogpr
Copy link
Owner

Can you resolve the conflict in build.sbt? Then I can merge it. Thanks!

@ghostdogpr ghostdogpr merged commit 8ce38b8 into ghostdogpr:master Aug 1, 2021
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