Skip to content
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

feat(core): add actor ID to DoneActorEvent, ErrorActorEvent #4942

Merged

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Jun 15, 2024

This changes DoneActorEvent (xstate.done.actor.*) and ErrorActorEvent (xstate.error.actor.*) to have an actorId prop which is equal to the ID of the Actor associated with the event.

Previously, this information was only available by extracting it from the type property of the event object using string operations.


Updated a test for DoneActorEvent to prove that actorId gets passed, but may want the same test for an ErrorActorEvent.

Ref: https://discord.com/channels/795785288994652170/1250894522616123494

Copy link

changeset-bot bot commented Jun 15, 2024

🦋 Changeset detected

Latest commit: 8edf73b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
xstate Minor

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

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I'm wondering if we should use actorId instead of id, since intuitively, id is self-referential (e.g. the id of the event) rather than something that references something else.

This changes `DoneActorEvent` (`xstate.done.actor.*`) and `ErrorActorEvent` (`xstate.error.actor.*`) to have an `actorId` prop which is equal to the ID of the Actor associated with the event.

Previously, this information was only available by extracting it from the `type` property of the event object using string operations.
@boneskull boneskull force-pushed the boneskull/actor-id-in-error-event branch from fc6fee0 to 689ebb6 Compare June 16, 2024 08:10
@boneskull
Copy link
Contributor Author

New prop is now actorId

@@ -3121,7 +3122,7 @@ describe('invoke', () => {
on: {
'*': {
actions: ({ event }) => {
actual.push(event.type);
actual.push(event as any);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't have to cast here, what is the reporter error without it?

Copy link
Contributor Author

@boneskull boneskull Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test machine does not declare its event types, so any event it handles will be of type AnyEventObject.
actual is DoneActorEvent<void>[], so we need a type assertion here. We could cast to DoneActorEvent<void>, but that seemed a little wordy. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, actual was string[], which did not need a type assertion.

use `AnyEventObject` instead of `DoneActorEvent` in `invoke.test.ts`

Co-authored-by: David Khourshid <davidkpiano@gmail.com>
@boneskull
Copy link
Contributor Author

@davidkpiano Suggestions committed

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to add a changeset (minor change: yarn changeset) 🚀

cc. @Andarist

@boneskull
Copy link
Contributor Author

@davidkpiano Is that something I should be doing?

@davidkpiano
Copy link
Member

@davidkpiano Is that something I should be doing?

Yes please

@boneskull
Copy link
Contributor Author

@davidkpiano Done. Hope I did that right. It looks like a minor bump to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants