[core] Transition function #4954
Conversation
🦋 Changeset detectedLatest commit: 4516833 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
Just a reminder to remove commented lines before merging 💜 |
packages/core/src/StateNode.ts
Outdated
| } | ||
| if (typeof action === 'function' && !('resolve' in action)) { | ||
| const type = `${this.id}|${kind}:${i}`; | ||
| this.machine.implementations.actions[type] = action as any; |
There was a problem hiding this comment.
- I don't particularly like that we are writing to this object because it might be shared among different machines in some way and we might potentially leak things between instances
- i'd be wary of the code here, I wonder if this gets "fixed" somehow but I think there might run into stale closure hazard by resolving this here and caching it
| if (typeof src !== 'string') { | ||
| this.machine.implementations.actors[sourceName] = src; | ||
| } |
There was a problem hiding this comment.
This leaks in cases like this:
const sharedActors = {};
setup({ actors: sharedActors }).createMachine({
invoke: {
src: fromPromise(async () => ""),
},
});
setup({ actors: sharedActors }).createMachine({
invoke: {
src: fromPromise(async () => 100),
},
});We should add a test case for this
| actorScope.defer(() => { | ||
| actorScope.system.scheduler.cancel(actorScope.self, resolvedSendId); | ||
| }); | ||
| actorScope.system.scheduler.cancel(actorScope.self, resolvedSendId); |
There was a problem hiding this comment.
do we have some new test that shows this is a desired change?
There was a problem hiding this comment.
There is no reason to defer it - it's a clean-up
| convertAction( | ||
| action as any, | ||
| snapshot.machine.root, | ||
| 'enqueue' + Math.random(), // TODO: this should come from state node ID which isn't provided |
There was a problem hiding this comment.
how hard would it be to fix it here?
packages/core/src/types.ts
Outdated
| export type SpecialExecutableAction = | ||
| | ExecutableSpawnAction | ||
| | ExecutableRaiseAction; |
There was a problem hiding this comment.
shouldnt this contain sendTo action too? they can also be delayed
| | SpecialExecutableAction | ||
| | (string extends TAction['type'] ? never : ToExecutableAction<TAction>) |
There was a problem hiding this comment.
| | SpecialExecutableAction | |
| | (string extends TAction['type'] ? never : ToExecutableAction<TAction>) | |
| | SpecialExecutableAction | |
| | (IsNever<TAction> extends true | |
| ? never | |
| : ToExecutableAction< | |
| string extends TAction['type'] | |
| ? { type: string & {}; params: NonReducibleUnknown } | |
| : TAction | |
| >) |
There was a problem hiding this comment.
we should add type tests for both setup and bare createMachine
| actorScope.actionExecutor( | ||
| { | ||
| type: builtinAction.type, | ||
| info: actionArgs, | ||
| params, | ||
| exec: () => {} // noop | ||
| }, | ||
| actorScope.self | ||
| ); | ||
| if (actorScope.self._processingStatus === ProcessingStatus.Running) { | ||
| builtinAction.execute(actorScope, params); |
There was a problem hiding this comment.
what happens here is beyond convoluted:
- a
noopexec gets passed intoactorScope.actionExecutor - then that gets mutated internally there for some actions (raise and sendTo)
- but then we can also learn that
actorScope.actionExecutorisn't entirely responsible for the actions' execution because we continue to callbuiltinAction.executehere
It's very confusing and hard to reason about as the logic is spread across those 2 places weirdly
| const resolvedTarget = | ||
| typeof target === 'string' | ||
| ? (source.getSnapshot() as AnyMachineSnapshot).children[target] | ||
| : target; | ||
|
|
||
| if (!resolvedTarget) { | ||
| throw new Error( | ||
| `Actor with id ${typeof target === 'string' ? target : target.sessionId} not found in the system.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
IMHO this doesn't fit here. It's forcefully trying to make something pass without considering what area of the code was designed to deal with this kind of resolving. Scheduler works with actor refs so even assuming here that the source is a machine actor is already a big stretch. The target should always come pre-resolved to the Scheduler as it was before.
| expect(spy).toMatchMockCallsInlineSnapshot(` | ||
| [ | ||
| [ | ||
| { | ||
| "counter": 1, | ||
| }, | ||
| ], | ||
| ] | ||
| `); |
There was a problem hiding this comment.
I just pushed out this test case that shows, what I believe to be, a regression. Note that I pushed out 2 test cases here: ece423d
They both are identical, the only difference is that one uses raise(...) whereas the other one uses sendTo(({ self }) => self, ...). While the raise one doesn't make a lot of sense, it clearly shows that something is broken here because those 2 tests are exactly the same if we consider raise to be a sugar for the other one.
This PR adds the
transition(…)function for returning a[snapshot, actions]tuple given the machine, state, and event.Its main use-case is for server-side workflows and a pure way of handling state transitions for actor logic.