Migrate the core module and tests to scala 3 #883
Conversation
|
The following releases can be used now: |
jchyb
force-pushed
the
scala-3
branch
2 times, most recently
from
126b1ae to
a72721c
Compare
|
I finally managed to fix all of the tests, with the one mentioned above being more important than I initially though. sangria-derivation module is also finished and working (with a small exception), but I think I should make a separate PR for that. |
jchyb
force-pushed
the
scala-3
branch
2 times, most recently
from
5b856b0 to
a718dc1
Compare
|
There are 2 scalac options that we should deactivate for scala3: Example on how to deal with that: https://github.com/sangria-graphql/sangria-spray-json/blob/main/build.sbt#L27 |
modules/core/src/main/scala/sangria/execution/QueryReducerExecutor.scala
Outdated
Show resolved
Hide resolved
|
The |
modules/core/src/main/scala-3/sangria/macros/ToExprGivens.scala
Outdated
Show resolved
Hide resolved
|
I had some trouble with disabling sangria-derivation compilation in Scala 3 (despite not having sources to compare to, mima plugin would still try to compile sangria-derivation), so for now I've removed Scala 3 from sbt-github-actions and set up simple separate GHA manually for scala 3 (via the ci-scala3.yml file). Later when PR'ing sangria-derivation I will remove that file and reenable the previous sbt-github-actions for scala 3 |
That's a reasonable approach for the time being! 👍 |
| sealed trait MappedAbstractType[T] extends Type with AbstractType with OutputType[T] { | ||
| sealed trait MappedAbstractType[T] extends Type with AbstractType { | ||
| def contraMap(value: T): Any | ||
| } |
There was a problem hiding this comment.
I see that this change causes binary compatibility errors (for scala 2). I removed OutputType[T] as it would cause compiler conflicts in scala 3 (there is another place where an anonymous class is created with both OutputType[T] from MappedAbstractType and OutputType[Any] from UnionType). Removing it here did not cause any problems elsewhere, so I did not think much of it. Since the this is a sealed trait inside a file full of them, making a version specific versions of the file seems impractical and heavy on the maintenance work... Outside of breaking binary compatibility the only solution I can think of is something akin to akka/akka-http#4113, where they have an sbt plugin that filters some implementations out of a single file depending on the scala version used
There was a problem hiding this comment.
You can also add some mimaBinaryIssueFilters.
|
For me, the PR is looking ready to merge to that we can iterate on that. Or do you still have something to change on it? |
|
Can you check is the publishing part would only publish the modules that are ready for scala 3? |
|
I think we can merge, anything else that I have not committed yet concerns sangria-derivation module, which is separate. About the publishing, the original GitHub action yml now does not have any mention of Scala 3 (as I purposefully removed it for now), and the new one I created does not have the publish action. I was planning to reenable the publish action via the original yaml when I PR sangria-derivation (which is ready, outside of some small issues which I am not sure I can fix) |
|
One more advantage of a separated GH action! Cool! |
All of the tests but one pass (the failing one isAll of the tests should pass now.deserialize automatically with spray-json JsonFormat (optional list with optional values and default)in FromInputSpec - I have not yet figured out why that is.For the code in the PR to work some dependencies need to be released for scala 3 and have their version updated. I commented near those dependencies with "TODO needs release", those releases should contain the following PRs to work:
While some changes to the codebase may seem questionable, they are the result of changed type inference, and, in a few cases, bugs:
Argumenttype parametersI imagine once those become fixed, the various workarounds used could be removed.
I still have to complete the sangria-derivation module to finish the migration, which may take some time. However the basic functionality of defining graphql code and executing queries should be already working.