-
-
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] Explicit spawn #3148
[v5] Explicit spawn #3148
Conversation
🦋 Changeset detectedLatest commit: 7e2c649 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 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7e2c649:
|
👇 Click on the image for a new way to code review
Legend |
Is there a reason for naming the config object |
Good feedback; open to renaming this to |
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@Andarist I reworked the actors to do the original thing - send events directly to the parent. It turns out that this was a better approach that avoided the complexity of dealing with observers and implicit observer method calls. This is also inspired by a relevant approach in Akka with dealing with futures (promises): https://blog.rockthejvm.com/pipe-pattern/ I think you'll appreciate this change 👍 |
case nextEventType: | ||
return event.data; |
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 is an unused code. It should either be removed or this event should be sent from here:
xstate/packages/core/src/actors.ts
Line 322 in d3274d1
self._parent?.send(toSCXMLEvent(value, { origin: self })); |
and the
self._parent.send(...)
should be performed here
origin: self | ||
}) | ||
); | ||
return event.data; |
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.
We are expected to return T
from here but this event.data
is basically any
/unknown
because it's a rejection value. We should return undefined
here:
return event.data; | |
return undefined; |
(and add a test for 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.
Curious on your thoughts here: since we want to represent an accurate promise snapshot, what do you think about representing promise snapshots as an "Either type" like other languages have?
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.
So based on T we'd type the actual snapshot as { ok: true, value: T } | { ok: false, error: unknown }
(or smth like that)?
self.send({ type: completeEventType }); | ||
} | ||
}); | ||
return state; |
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.
return state
is technically correct within this whole behavior but in practice, this is return undefined
. I wonder if it's worth to change that to make this more visible - right now a reader could assume that some state can be associated with this behavior.
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.
Should I rename state
to snapshot
to make this more clear?
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.
That could be done (i dont mind using state here though). The main point was that this particular behavior wont have non-undefined state/snapshot and when reading through those return statements it looks like it can
…undant code (#3319) * Fixed `ObservableActorRef` subscriptions and remove some outdated/redundant code * Add changesets
80785fc
to
8655dad
Compare
Congratulations! Could we expect the v5 to be published soon to the |
It's already available as |
Awesome, thank you very much for publishing it! |
This PR makes
spawn
more explicit and introduces the following changes:spawn
function and relatedCapturedState
code has been removedassign
actions now comes from the 3rd "meta" argument:fromObservable
deliver values rather than events:This PR closes STA-1269