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

Move circe codecs to the core module #102

Merged
merged 1 commit into from
Dec 1, 2019
Merged

Conversation

rtimush
Copy link
Contributor

@rtimush rtimush commented Nov 29, 2019

Circe codecs for GraphQLRequest and GraphQLResponse are generally useful, so I moved them out of the http4s integration module. I also made GraphQLRequest public.

The pull request uses the trick described in https://blog.7mind.io/no-more-orphans.html — circe dependency is marked as optional, so you don't need circe to use caliban, but if you have it, the typeclass instances will be available without additional imports.

@rtimush rtimush force-pushed the circe branch 2 times, most recently from 7cb4467 to f9de6f2 Compare November 29, 2019 21:56
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.

Looks great! I just have a suggestion to possibly make the dependency even more minimal.


private object GraphQLRequestCirce {
import io.circe._
import io.circe.derivation._
Copy link
Owner

Choose a reason for hiding this comment

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

Since we are using circe-derivation only for this single (and simple) type, how about deriving it manually and only depend on circe-core?
Because not everyone uses circe-derivation, some people use circe-generic, others circe-magnolia. That way we would keep the dependency minimal.

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 think codecs created by circe-derivation macros depend on the circe core only, so people won't need circe-derivation as a dependency. Although, to be on the safe side I can indeed implement it manually.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh yeah, you must be right! All good then.

@@ -103,7 +104,6 @@ lazy val http4s = project
"org.http4s" %% "http4s-circe" % "0.21.0-M5",
"org.http4s" %% "http4s-blaze-server" % "0.21.0-M5",
"io.circe" %% "circe-parser" % "0.12.3",
"io.circe" %% "circe-derivation" % "0.12.0-M7",
Copy link
Owner

Choose a reason for hiding this comment

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

If we reduce the dependency to circe-core as I suggested in the other comment, this is good. But otherwise, I think we should keep that one so that people who use caliban-http4s don't need to add the dependency to circe-derivation themselves.

@ghostdogpr ghostdogpr merged commit cbf1496 into ghostdogpr:master Dec 1, 2019
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