evolution-gaming / akka-effect

Cats-Effect & Akka interop
MIT License
55 stars 5 forks source link

refactor non-public API to be more aligned with Akka types #302

Closed dfakhritdinov closed 6 months ago

dfakhritdinov commented 6 months ago

Akka Persistence API (used by the lib) is not typed and relies on underline persistence plugin implementation.

I see not much use in having types & casts that will be set to Any later, thus dropping types is beneficial due to no casts like A -> Any -> A. Code that uses the persistence must by itself somehow interpret persistence plugin data having no type hints (already implemented in PersistentActor or its replacements)

mr-git commented 6 months ago

otherwise it looks OK for me, though I lack deeper knowledge on how this is used in other projects, thus I cannot predict full extend of implications

rtar commented 6 months ago

Let's discuss this.

I actually see an important value in using A despite Any being the only type passed there.

I.e. let's imagine a simplified class like following:

class Service[A] {
  def save(a: A): Unit = {
    println(a)
    register(a)
  }
  def register(a: A): Unit = {
    sendRegistration(a)
  }
}

Even if the only value we pass as A is Any it ensures that register method is only called with the same object as passed in save.

If we remove A we may have the bugs like following, and compiler will not prevent them:

class Service {
  def save(a: Any): Unit = {
    println(a)
    register(7)
  }
  def register(a: Any): Unit = {
    sendRegistration(a)
  }
}

It is just too easy to pass a different value to an argument where Any is expected.

It does not mean that the code you have changed could not be simplified somehow, of course, but there is a value in that type parameter, I think.

dfakhritdinov commented 6 months ago

@rtar , thanks for sharing your vision. I agree that types provide some safety even been erased later by setting them to Any.

My change has a goal of aligning types with ones from Akka, reducing casts and related with them constraints. And in some places, like EventSourcedPersistence, moving types on higher level thus making them useful without casting.

Lets take a look on SnapshotStoreInterop where snapshot type was replaced with Any. The class implements SnapshotStore (properly typed btw) for Akka Persistence, API that operates with Any as snapshot type. I do not think that keeping A in the signature will somehow benefit the class because the implementation would always cast Any to A. I see not much effort in such casting thus prefer to have type of Akka snapshot in the class signature. I see your point in API safety but prefer avoiding misleading code and cast Any -> A more than that. Same explanation actual for EventStoreInterop.

EventSourcedPersistence.fromAkkaPlugins lost its type params due to changes in EventStoreInterop & SnapshotStoreInterop, I see not much to discuss here.

EventSourcedActorOf is bit more complicated case but I try to elaborate it as well. The class is a factory for building "persistent" actor from persistence and actor's logic typed previously as Lifecycle[F, S, E, C] and changed to Lifecycle[F, Any, Any, Any]. My reasoning here again was in avoiding misleading types that are never enforced by always been Any. Lets see simplified signature of ActorOf (factory that actually creates instance of Akka Actor) first:

object ActorOf {

  def apply[F[_]: Async: ToFuture](
    receiveOf: ActorCtx[F] => Resource[F, Envelope[Any] => Boolean]
  ): Actor = ???

}

The signature explicitly saying that incoming message is of type Any thus actor's logic must deal with its parsing/decoding by itself. I see having the logic typed as Lifecycle[F, S, E, C] (where C is the command) misleading, because no actual type constraints present. Having Lifecycle[F, Any, Any, Any] explicitly says that one passing the logic must take care of extracting C from Any instead of relying on some types.

Other two dropped type params, S and E refers to snapshot and event from Akka Persistence. As I have pointed earlier, both of then are Any and I prefer to put it explicitly as is.

Dropping types let me avoid casting Any to C and misleading API that looks typed but is not. Please check PersistentActorOf as currently used wrapper on top of Persistent Actor, thing to be replaced by EventSourcedActorOf.

Will we use new fully typed persistent implementation later?

Yes, later one can implement for example Kafka-Journal integration in terms of typed SnapshotStore and EventStore, effectively moving parsing/decoding logic out of actor's logic Lifecycle[F, Any, Any, Any] thus making EventSourcedActorOf be typed again, but its separate story impossible for now.