-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[v5] Remove interpreter.children
#2840
Conversation
🦋 Changeset detectedLatest commit: fc5ca7b The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
if (!child) { | ||
return; | ||
} | ||
|
||
this.children.delete(childId); | ||
this.forwardTo.delete(childId); | ||
delete this.state.children[childId]; |
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 probably should be refactored to avoid mutation of this.state
- I think most people treat this as immutable. Maybe grabbing the actor ref to stop within machine APIs~ and embedding that in the action would help? This way we wouldn't have to access this here by ID
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.
Let's address this in a separate PR. This isn't part of this PR's changes.
The tricky thing is that the state.children
then needs to change as part of the stop(...)
action, and currently, actions can only affect the context
, and not other parts of the state, so this requires a lot more thought. I agree it should change, though.
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.
Do we have any ticket anywhere to track this?
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.
Let's not forget about a changeset here - this is a pretty big user-facing change. |
This PR removes
interpreter.children
in favor ofinterpreter.state.children
(redundant).