caliban icon indicating copy to clipboard operation
caliban copied to clipboard

Unify definition of custom schemas for Scala 2 & 3

Open kyri-petrou opened this issue 1 year ago • 5 comments

One of the confusing things in Caliban at the moment (both for new users following the documentation and Scala 2 -> 3 migrations) is that due to differences in implicit resolution between the two Scala versions, in Scala 2 user-defined schemas require to extend GenericSchema whereas in Scala 3 they need to extend SchemaDerivation.

To make things worse, the user code might actually work in simple cases if the extend GenericSchema, but as the codebase grows, experience significantly increased compilation times or compiler errors that have nothing to do with their recent changes / additions.

To improve UX on this front, this changes implements things a bit differently:

  1. Adds a SchemaInstances trait, which contains all the implicits previously defined in GenericSchema.
  2. Schema companion obj now extends SchemaInstances and SchemaDerivation[Any] (instead of GenericSchema)
  3. GenericSchema in Scala 2 extends SchemaInstances with SchemaDerivation[R]
  4. GenericSchema in Scala 3 extends SchemaDerivation[R] only
  5. Extending SchemaDerivation in Scala 3 now raises a deprecatedInheritance warning

This means that Scala 3 users can now simply extend GenericSchema instead of SchemaDerivation. Note that this change might cause some compilation errors if users were using a method that is now defined within SchemaInstances, but that should be very easy to fix using one of two ways:

Before:

object MySchema extends GenericSchema[Foo] {
  // ...
}

After:

// Recommended
import Schema.*
object MySchema extends GenericSchema[Foo] {
  // ...
}

// Not recommended but matches previous behaviour
object MySchema extends SchemaInstances, GenericSchema[Foo] {
  // ...
}

Given that adding an import to make this work is a very simple thing (IDEs will even recommend that to you) and that we can easily document that in the release notes, I think this is a small price to pay for a better long-term UX.

Thoughts?

kyri-petrou avatar Aug 10 '24 08:08 kyri-petrou

Note that I published this locally and tested it on 2 fairly large GraphQL APIs at $WORK and the only change necessary were 2 LOC to extend GenericSchema instead of SchemaDerivation due to the deprecatedInheritance compiler warning. No other changes were necessary.

kyri-petrou avatar Aug 10 '24 08:08 kyri-petrou

What is the issue if GenericSchema extend SchemaInstances with Scala 3?

ghostdogpr avatar Aug 12 '24 01:08 ghostdogpr

Shouldn't import Schema.* be import Schema.givens?

Also Before was object MySchema extends SchemaDerivation[Foo] {, no?

ghostdogpr avatar Aug 12 '24 01:08 ghostdogpr

What is the issue if GenericSchema extend SchemaInstances with Scala 3?

From experience, I found that it works in most cases, but it's generally slower to compile (due to resolving implicit ambiguity), and in some cases it fails altogether, e.g., https://scastie.scala-lang.org/ziVxfOMkSOeGplO0MYUbJw

Shouldn't import Schema.* be import Schema.givens?

No because the implicits in Schema are defined as implicit def. the .given import is needed only if the implicits are defined as given foo = ???

Also Before was object MySchema extends SchemaDerivation[Foo] {, no?

Sorry that was a bit confusing. What I meant was, if users were using extends GenericSchema[Foo] in Scala 3, there is a small chance they'll now get a compiler error if they were not importing Schema.*.

If they were using extends SchemaDerivation, now they'll get a compiler warning and they'll change it to extends GenericSchema, but they won't need to do any other changes (no imports or otherwise required)

kyri-petrou avatar Aug 12 '24 03:08 kyri-petrou

Ah okay, so in the general case, object MySchema extends GenericSchema[Foo] should be all we need.

Thanks for clearing things up, LGTM.

ghostdogpr avatar Aug 12 '24 04:08 ghostdogpr

After giving it some more thought and trying a few things out, I think that the changes in this PR will no longer be an issue in Scala 3.7+ (or 3.5+ with the -source: future scalac flag).

I'll close it for now and we can re-evaluate in the future whether it's needed or not

kyri-petrou avatar Aug 31 '24 11:08 kyri-petrou