feat: 🚀 Provide Play-JSON body encoder #114
Conversation
|
nice 👍 |
| ) | ||
|
|
||
| val playJson: Seq[ModuleID] = Seq( | ||
| "com.typesafe.play" %% "play-json" % _playJson, |
There was a problem hiding this comment.
we should probably make this % Provided, otherwise we lock the user into a version
There was a problem hiding this comment.
This doesn't lock user in a version, they can still use another version in their codebase (direct dependency or using dependencyOverrides) as long as the version is compatible.
By the way, recent versions of sbt will let users know if they are pulling 2 incompatible (according to semver) versions.
If we put it to scope Provided and users use a version incompatible with what we expected in pact4s, it would break as well.
There was a problem hiding this comment.
@gaeljw right -- it's just that this is what the Provided scope is for, it requires the user to include this library and include a compatible version of play JSON -- rather than including one by default which needs to be excluded if the user wants to use a different version (or being evicted if they include a version in addition to the default one). It's just cleaner, IMO, and also a pattern we already use in this library.
There was a problem hiding this comment.
I don't see any downside of using the Compile scope: on the contrary it will let sbt highlight potential incompatibilities. I might be wrong or partial but Provided scope is more for context like Tomcat or Spark where the runtime server bring the dependencies itself. If your code needs play-json it must declare it needs it IMHO.
Anyway I'll adapt to the standards/pattern of this library if needed :)
Though it's already merged and circe module is setup the same way: https://github.com/jbwheatley/pact4s/blob/main/project/Dependencies.scala#L50
There was a problem hiding this comment.
If someone is bringing this library into their project, they will already have a version of play-json defined. Since you include it in the Compile scope this means:
- there are now potentially two versions included, one being evicted
- they will need to remove their existing inclusion (may not be feasible depending on project setup)
- they will need to exclude the version included by the pact4s library
I don't see any benefit to including it in Compile scope, personally. In fact, your example is the only other place where we don't include the dependency as Provided. Not a hill I'm going to die on, maybe @jbwheatley can weigh in.
There was a problem hiding this comment.
PS, I don't think there really is a right answer to this, it just depends on the dependency strategy we want to take for this library. We can either include everything in Compile scope and let the users handle potential evictions/conflicts, or we can use Provided scope and force users to explicitly include a compatible version of the dependency, which I prefer simply because I think it's less complicated.
There was a problem hiding this comment.
Yeah, I totally understand your point. I guess it's a matter of habits/taste as well. I'll be fine with any choice :)
This PR brings supports for Play-JSON in addition to Circe as the JSON library for encoding/decoding.
I basically copy/pasted (tests included) what's inside the pact4s-circe project and adapted it to Play-JSON.
This will close #10