-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Unify ActorRef type #2275
Conversation
🦋 Changeset detectedLatest commit: 219e80b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
What is the guidance about It seems to be somewhat XState specific, rather than actor-specific. |
The type TEvent = { type: 'one' } | { type: 'two' }
type TActor = BaseActorRef<TEvent>
const baseActor: TActor = null!
// Argument of type 'TActor' is not assignable to parameter of type 'BaseActorRef<EventObject, any>'.
// Type 'EventObject' is not assignable to type 'TEvent'.
// Type 'EventObject' is not assignable to type '{ type: "two"; }'.
// Types of property 'type' are incompatible.
// Type 'string' is not assignable to type '"two"'.(2345)
const a = toActorRef(baseActor) |
extends ActorRef<TEvent, TEmitted> { | ||
export interface ActorRef<TEvent extends EventObject, TEmitted = any> | ||
extends Subscribable<TEmitted> { | ||
send: Sender<TEvent>; | ||
id: string; | ||
getSnapshot: () => TEmitted | undefined; | ||
stop?: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q for v5: should we kill .stop
on ActorRefs? should such direct call only be allowed for parents? being able to stop an arbitrary actor when the ref gets shared seems to be dangerous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to stop an actor from any other actor that has a ref to it. This matches the behavior in Akka; I don't see how this can be dangerous (obviously, it's the responsibility of the developer to stop it properly): https://alvinalexander.com/scala/how-to-stop-akka-actors-in-scala-gracefulstop-poisonpill/ Okay maybe not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO there are two dangers here:
- it's an imperative/eager call, we should prefer pure actions
- that actor may live both in contexts of different actors and also in internal structures of its parent, especially for the latter we can't just shift the responsibility of handling that to the user as they don't control that at all
According to the Akka docs we can find this:
Classic actors can be stopped with the stop method of ActorContext or ActorSystem. [...] There is also a stop method in the ActorContext but it can only be used for stopping direct child actors and not any arbitrary actor.
and this:
Typically the context is used for stopping the actor itself or child actors and the system for stopping top level actors
So it seems to me that stopping the arbitrary actor might not be a recommended pattern in Akka, even if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but let's discuss in a separate PR, as this might be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right; we can probably rename this to |
Fixed in 15c6e7d (I believe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change affects public interfaces from the types perspective so adding some changeset about this would still be a good thing to do so people get aware of the change if it affects them anyhow
This PR unifies the
ActorRef
type to contain all methods previously only present on theSpawnedActorRef
type. This was found to be redundant in #2271, so now the types are as follows:ActorRef
contains all methods (required)BaseActorRef
only requiressend
; all other methods are optional. This should seldom be used.SpawnedActorRef
is now an alias toActorRef
; it is deprecated and will be removed in v5.