feat: Replace jsoniter macros with circe #83
Conversation
| @@ -33,7 +33,7 @@ val commonSettings = Seq( | |||
| "com.disneystreaming" %%% "weaver-cats" % "0.8.4" % Test | |||
| ), | |||
| mimaPreviousArtifacts := Set( | |||
| organization.value %%% name.value % "0.0.7" | |||
There was a problem hiding this comment.
question: does the bincompat breakage affect langoustine? conversely, are the codec instances even used / need to be visible outside of jsonrpclib?
There was a problem hiding this comment.
good question, I checked it manually and it seems that there is one place where it does:
[error] -- [E172] Type Error: /home/kghost/workspace/langoustine/modules/tracer/frontend/src/main/scala/component.jsonviewer.scala:63:44
[error] 63 | displayJson(ep, mode.signal, modalBus)
[error] | ^
[error] |No given instance of type com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[
[error] | jsonrpclib.ErrorPayload] was found for a context parameter of method displayJson in package langoustine.tracer
and there is another one in tests:
[info] compiling 17 Scala sources to /home/kghost/workspace/langoustine/modules/tests/target/jvm-3/test-classes ...
[error] -- [E172] Type Error: /home/kghost/workspace/langoustine/modules/tests/src/test/scala/testkit.scala:95:60
[error] 95 | upickle.default.read[req.Out](writeToArray(outc.encode(res)))
[error] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error] |No given instance of type com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[jsonrpclib.Payload] was found
[error] one error found
There was a problem hiding this comment.
should we make the circe codecs package private?
There was a problem hiding this comment.
sigh... removing jsoniter macros was supposed to help us in the compilation flakiness 😓
not sure about circe codecs. I can see it being useful for tests (like what langoustine did), but the tracer's usecase should probably be supported with something more high-level
| @@ -1,6 +1,9 @@ | |||
| package jsonrpclib | |||
|
|
|||
| import com.github.plokhotnyuk.jsoniter_scala.core._ | |||
| import com.github.plokhotnyuk.jsoniter_scala.circe.JsoniterScalaCodec._ | |||
| import io.circe.Json | |||
| import io.circe.{Encoder, Decoder} | |||
|
|
|||
| trait Codec[A] { | |||
There was a problem hiding this comment.
Not sure this construct makes sense if we standardise on circe, to be honest ... maybe it should go altogether
There was a problem hiding this comment.
I gave it a quick shot in the last commit. There might be some small differences regarding optional input handling because I wanted to just try it out. Let me know what do you think and I can either polish it or just revert.
| def apply(a: A): Json = | ||
| documentToJson(Document.encode(a)) |
There was a problem hiding this comment.
an alternative to this would be to actually have a first class support for circe in smithy4s. But this seems to be quite a big increase in terms of maintenance. Also, I am not sure if doing it 100% with circe would actually bring any performance gains as here the heavy lifting is handled by jsoniter.
There was a problem hiding this comment.
as discussed in DMs I think smithy4s-circe is unmaintainable, besides it's effectively the same as our Document codecs except with a different target. Something that we'd get "for free" if we had disneystreaming/smithy4s#1659
for now I would keep the Blob / Array[Byte] as it doesn't have the same problem, and is (possibly) handled by jsoniter by copying a contiguous array chunk rather than creating an in-memory AST...
and Document.encode means we need to involve a cache, presumably the global one
There was a problem hiding this comment.
@Baccata what do you think? Do we keep it like that with the intermediate translation via document or should it be reverted back to Array[Byte]?
There was a problem hiding this comment.
I mean, besides the fact that the schema is implicit, I'm fine with the current code living here. I don't want to maintain a smithy4s-circe module
| .map(fromJson) | ||
| .flatMap(Document.decode[A]) | ||
| .left | ||
| .map(e => DecodingFailure.apply(DecodingFailure.Reason.CustomReason(e.getMessage), c)) |
There was a problem hiding this comment.
You can probably probably recreate the circe history as well
This change is binary incompatible due to removal of fields that used to hold jsoniter codec instances.