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

Fixed an issue with not being able to send events to initially started child actors when using predictableActionArguments #3549

Merged
merged 1 commit into from Aug 26, 2022

Conversation

Andarist
Copy link
Member

fixes #3548

…d child actors when using `predictableActionArguments`
@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2022

🦋 Changeset detected

Latest commit: 0f1328c

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

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

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

@ghost
Copy link

ghost commented Aug 26, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

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 0f1328c:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration
gifted-dewdney-yg4mee Issue #3548

@@ -810,7 +810,8 @@ export class Interpreter<

private sendTo = (
event: SCXML.Event<AnyEventObject>,
to: string | number | ActorRef<any>
to: string | number | ActorRef<any>,
immediate: boolean
Copy link
Member Author

@Andarist Andarist Aug 26, 2022

Choose a reason for hiding this comment

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

The problem here was that executing initial actions is still done using Interpreter#execute - and doesn't happen from within StateMachine#transition:

// Execute actions
if (
(!this.machine.config.predictableActionArguments ||
// this is currently required to execute initial actions as the `initialState` gets cached
// we can't just recompute it (and execute actions while doing so) because we try to preserve identity of actors created within initial assigns
_event === initEvent) &&
this.options.execute
) {
this.execute(this.state);
} else {
let item: typeof this._outgoingQueue[number] | undefined;
while ((item = this._outgoingQueue.shift())) {
item[0].send(item[1]);
}
}

So initial send actions were never sent out after my recent PR: #3540 where I've introduced _outgoingQueue

@@ -991,7 +992,7 @@ export class Interpreter<
return;
} else {
if (sendAction.to) {
this.sendTo(sendAction._event, sendAction.to);
this.sendTo(sendAction._event, sendAction.to, _event === initEvent);
Copy link
Member

Choose a reason for hiding this comment

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

Was the problem that it was not sent immediately, or not sent at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

they would be flushed within the next service.send(ev) - which could happen at any time, or not at all.


const service = interpret(machine).start();

expect(service.getSnapshot().value).toBe('done');
Copy link
Member

Choose a reason for hiding this comment

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

In real-life scenarios, we can't assume that this is synchronous. Will the machine still get the PONG event after it settles on the initial state (it should)?

Maybe we can test this by instead listening for .onDone(done) so we don't depend on it being sync 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

In real-life scenarios, we can't assume that this is synchronous. Will the machine still get the PONG event after it settles on the initial state (it should)?

Yes, in this case, the test case is really more about the child receiving an event than about the parent receiving the response - I could slim it down.

Maybe we can test this by instead listening for .onDone(done) so we don't depend on it being sync 🤔

I agree that those conceptually are async. However, I think that we also need to test synchronous logic as sync code often exposes more edge cases in scheduling etc. So it feels more beneficial to prefer sync tests in our test suite when possible - despite the mental model being async.

@Andarist Andarist merged commit 768c4e9 into main Aug 26, 2022
@Andarist Andarist deleted the fix/3548 branch August 26, 2022 12:47
@github-actions github-actions bot mentioned this pull request Aug 26, 2022
davidkpiano added a commit that referenced this pull request Aug 27, 2022
davidkpiano added a commit that referenced this pull request Jan 3, 2023
* WIP

* WIP types

* Fix sendParent weird issue

* service.state -> service.getSnapshot()

* Move symbolObservable

* Skip sync, change default sessionId to undefined, add actorCtx to StateMachine

* Move logic to state

* Make stopping more general

* Add getInitialState to behavior type

* Fixing types WIP

* Use strings instead of symbols

* Cache initial state

* Delete ObservableActorRef

* Add getActions to Behavior

* Move state-specific stuff outside of interpreter WIP

* Make defer public (temporarily)

* Fix types

* Fix types

* Skip failing tests for now

* Move stopping logic

* Add internal state to behaviors

* Fixing tests WIP

* Fix a test

* Big refactor; only 2 tests left to fix!

* Almost there...

* Skipping tests

* Fix types

* Cleanup

* Cleanup

* Unskip a couple tests

* Update packages/core/src/interpreter.ts

* Simplify interpreter actorContext

* Move machine-specific code to exec.ts

* Cleanup

* Add test from #3549

* Add failing test

* interpreter.machine -> interpreter.behavior, fix tests caused by invalid cached initial state

* Lint

* Remove fromMachine

* Re-enable predictableExec test

* Fix memory leak

* Inline stopStep

* Extract machineMicrostep (bikeshed)

* Make resolveStateValue part of machine; machine.microstep state must be State instance

* Add get state.meta, get state.tags, start to remove State.create in favor of machine.createState

* WIP simplifying stateUtils (only need historyValue, not state for some functions) and getting rid of State.from

* WIP Make state.machine required

* Remove machine argument from macroStep

* More redundant machine removal

* More redundant machine removal

* More cleanup

* Remove matchState

* Compute value from configuration

* Refactoring WIP

* Add state.clone

* Simplify microstep

* Fix use of null

* More cleanup

* Linting

* Fix linting in xstate/inspect

* More linting

* Remove interpreter.listeners

* Sigh, the comment is right

* Move more functions internal

* Move exitStates

* Make selectEventlessTransitions internal

* Add AnyTransitionDefinition and AnyHistoryValue

* Scope actions

* Fix history

* Use Array.flatMap and .flat

* Optimization: use for..of instead of .forEach

* Cleanup types

* Restore state.meta in JSON

* Remove unnecessary generic types

* Cleanup types

* Fix use of observers

* Remove unused `empty` from a test file

* Update packages/core/src/StateNode.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/actions/send.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/actions/send.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Fixed `AnyBehavior` to actually allow any `Behavior` (#3579)

* Resolve action objects as lazily as possible to avoid stale closure problem in React (#3581)

* Remove `Interpreter#nextState` (#3580)

* Cleanup redundant code related to `execAction`

* Remove `Interpreter#nextState`

* Removed unused variables

* Fixed looking up inlined action in the implementations objects (#3582)

* Fixed looking up inlined action in the implementations objects

* Fixed test titles

Co-authored-by: David Khourshid <davidkpiano@gmail.com>

* Add changeset

Co-authored-by: David Khourshid <davidkpiano@gmail.com>

* Stop tracking `preservedContexts`, only the last one is relevant anyway

* Dont wrap non-resolvable action objects in `ExecutableAction`

* Remove uncalled code

* Improve error message

* Don't use `null` as initial event

* Lint

* Resolve event earlier

* Rename machine.key -> machine.id

* Add TODO for explicit type for `output`

* Machine is required; fix up check in output

* Fix this.actions on State

* Make exec required

* Revert ActorRefFrom typing

* Update packages/xstate-vue/test/UseActor.vue

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Use `ActorRefFrom` over `InterpreterFrom` in a couple of places

* Drop redundant casts to `StateFrom`

* Update packages/core/test/types.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Remove `state.inert`

* Remove State.create

* Remove outdated comment

* Add changeset for removal of `matchState(...)`

* Remove unnecessary cast

* Remove unused `StateFrom` import

* Remove traces of the `StateNodeConfig["key"]`

* Add ActorRef.id, remove ActorRef.name

* Update packages/core/src/interpreter.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Fix imports

* Update packages/core/src/interpreter.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/interpreter.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/interpreter.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/stateUtils.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/interpreter.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Reflect the type for internal `Interpreter#observers` closer to the possible runtime values (#3593)

* Simplify `transitionParallelNode` a little bit (#3591)

* Simplify `transitionParallelNode` a little bit

* Rename variables

* Handle events sent to `self` closer to events send to other targets (#3592)

* Refactor deferred

* Fix execution issue

* Cleanup

* Cleanup

* Cleanup

* Remove Interpreter.doneListeners and Interpreter.stopListeners

* Add getStatus to behavior to detect when done

* Remove Interpreter.onStop

* Use getStatus to remove state-specific done checking

* Remove machine check from `interpret` fn

* Remove errorListeners and .onError()

* Remove state check from forwarding

* Add restoreState

* Move error throwing to state machine behavior

* Lint

* .initialState -> .getInitialState(...)

* Remove `strict`

* Refactoring to avoid mutation part 1

* macrostep now returns multiple states

* Refactoring microstep cont'd

* Clean up microstep tests

* Add test

* Add preserved actions test

* .initialState -> .getInitialState()

* Fix types

* Cleanup

* Cleanup

* Fix: memo eviction

* Add behaviors.test.ts

* Move tests

* Cleanup

* Fix tests

* Fix tests

* Deprecate toActorRef

* Remove `Interpreter#onError` (#3691)

* Update .changeset/green-brooms-laugh.md

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Externalize functions

* Remove import

* Update packages/core/src/actions/send.ts

* Adjust configuration types

* Provide precomputed `State["done"]` and `State["output"]` (#3710)

* Update packages/core/src/StateMachine.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Address getInitialState comment

* Actor observer impl refactor WIP

* Cleanup observable handling in fromObservable

* Finish observer handling

* Update packages/core/src/StateMachine.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Make cloneState a "private" function

* Update packages/xstate-vue/test/UseActorSimple.vue

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/xstate-vue/test/UseActorSimple.vue

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/xstate-inspect/src/server.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/xstate-inspect/src/browser.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Revert temporary changes in a test file

* Allow behaviors and behaviorCreators in provided actors object

* Do todos

* bring back remived comments in a test file

* Simplify some tests

* inline `infinite$` observable in the remaining call sites

* rename a test block

* add a changeset about `off` removal

* Localice `createMachine` per test in one of the test files

* Remove `spawn`+`autoForward` tests (#3718)

* Remove `sync` option (#3717)

* Remove `sync` option

* remove more leftovers

* remove another leftover

* remove another leftover

* Enable one of the skipped tests

* Update packages/core/src/utils.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Remove `ActorContext['observers']` (#3722)

* Update packages/core/src/stateUtils.ts

* reuse existing local variable

* Update packages/core/src/stateUtils.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/stateUtils.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Avoid double traversal when adding ancestors in `getConfiguration`

* Update packages/core/src/stateUtils.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Remove underscore convention

* Refactor

* Fix type comment

* Unused type

* Initial state shouldn't be memoized (TODO: fix React tests)

* Attempt to fix some React tests

* Cleanup

* Defer stopping actors (#3719)

* Remove a custom `done` callback from the `Observer` (#3720)

* Remove a custom `done` callback from the `Observer`

* Spike removing the leftover dependencies on done listeners

* [v5] Simplify `interpreter.send(...)` method (#3568)

* Simplify function signature of `interpreter.send(eventObj)`

* Remove unused types

* Add changeset

* Cleanup types

* Simplify more event-related methods and functions (#3570)

* Add changeset for `@xstate/fsm`

* Fix tests

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* StateMachine implements Behavior

* Rename defer -> delaySend

* [v5] Refactor actions (#3726)

* Test cleanup

* Committing this just to checkpoint it

* Add status, change Actorcontext.exec sig

* Actions resolve to tuple

* Declassify ExeuctableAction

* Goodbye ExecutableAction

* Goodbye exec.ts

* Refactor createDynamicAction

* More refactoring

* Yay more deleting

* More refactoring

* exectute2 -> execute

* Enable root initial actions (addresses #466)

* Working through types

* Still working on types

* Sorry, getting a little messy again

* Revert "Sorry, getting a little messy again"

This reverts commit a609de8.

* Fix test

* Ensure no side-effects in getInitialState

* I guess this makes sense

* Started state should come from behavior.start if it exists

* Get rid of state-specific stuff

* Remove test

* Update packages/core/test/predictableExec.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Refactor test

* Update packages/core/test/actions.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/test/actions.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/test/predictableExec.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Remove unused import

* Update packages/core/test/actions.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/test/actions.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Interpreter tests

* Remove unused `ActorContext['exec']` (#3734)

* Fixed initial state cache eviction in the `Interpreter` (#3733)

* Tweak logic around stopping actors (#3737)

* Tweak logic around stopping actors

* remove unused imports

* Reuse the `ActorStatus` enum in the `handleAction`

* Tweak types

* Free the stopped `sessionId` in the registry

* Make `getInitialState` in the `Interpreter` private (#3738)

* Update packages/core/test/interpreter.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/core/src/stateUtils.ts

* Small cleanup of internal implementations of actors (#3740)

* Small cleanup of internal implementations of actors

* Remove redundant completion notifications sent out by some actors

* Wrap sending `xstate.snapshot.${id}` events with `defer`

* Drop the unused "next" case from the event observable

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Add a test for unique promises created by the same behavior

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
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.

Bug: invoked callback not working with predictableActionArguments
2 participants