feat: Replace jsoniter macros with circe #83
Conversation
| "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
| 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
There was a problem hiding this comment.
alright then, let's roll with this
modules/smithy4s/src/main/scala/jsonrpclib/smithy4sinterop/CirceJson.scala
Outdated
Show resolved
Hide resolved
modules/examples/client/src/main/scala/examples/client/ClientMain.scala
Outdated
Show resolved
Hide resolved
| case PayloadPath.Segment.Label(name) => CursorOp.DownField(name) | ||
| case PayloadPath.Segment.Index(i) => CursorOp.DownN(i) |
There was a problem hiding this comment.
I hope it works :D
modules/core/src/main/scala/jsonrpclib/internals/MessageDispatcher.scala
Outdated
Show resolved
Hide resolved
This change is binary incompatible due to removal of fields that used to hold jsoniter codec instances.