[v5] Interpret all behaviors

[davidkpiano]

The biggest changes:

• ~~ObservableActorRef~~ is removed
• interpret now works with any Behavior: interpret(behavior)
• The Behavior interface now has Behavior.getSnapshot, which is important for determining which "state" should be exposed, and which state is internal state
• predictableActionArguments flag is removed; actorCtx.exec is used instead
• Most state-machine-specific code is removed from the Interpreter; still have some more work to do here

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: fc3ca7253da6cb73b5c1d877cbed0fb498f6998e

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

This PR includes changesets to release 4 packages

Name	Type
xstate	Major
@xstate/fsm	Major
@xstate/react	Major
@xstate/vue	Major

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

[codesandbox-ci[bot]]

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 fc3ca7253da6cb73b5c1d877cbed0fb498f6998e:

Sandbox	Source
XState Example Template	Configuration
XState React Template	Configuration

[ghost]

👇 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!

Legend

[davidkpiano]

~~@Andarist There are 2 tests in @xstate/react only failing; can you help me investigate those?~~ EDIT: resolved

[davidkpiano]

@Andarist Some conflicts, can you help with those?

[Andarist]

@davidkpiano ye, i can handle those conflicts right before the final merge

[davidkpiano]

Re: .start(initialState), I agree with deprecating it, and propose the idea that if a machine's behavior will change due to some preliminary data (such as input, restored state, or custom implementations), then the machine should be "extended" instead of options being passed to the interpreter.

We already do this with e.g. actions, guards, etc. via machine.provide(...), and we can do the same for starting at a specific state:

-interpret(machine).start(restoredState);
+interpret(machine.at(restoredState)).start();

Something like machine.at(restoredState) (bikeshed) can also do verification in the future that the restoredState is a valid state for the machine. This shouldn't be the interpreter's responsibility.

[davidkpiano]

@Andarist This is ready for final review pending merge conflicts

[Andarist]

1. I feel that it's crucial for this PR to make StateMachine implement the Behavior interface. As long as it doesn't implement it, it feel like interpret isn't actually accepting Behavior - it's accepting Behavior | StateMachine, and thus the unification process of all the things didn't actually succeed. I have a comment that goes into this a little bit further here
2. with interpret accepting any Behavior we end up with some problems in @xstate/react etc. Its useInterpret has some very machine-oriented options (and it's still accepting TMachine and not TBehavior). That probably can be handled in a separate PR but it's worth noting here.
3. There is one minor unanswered comment here
4. Behavior['getInitialState'] is calling factories embedded in behaviors and those are usually side-effectful. This is a big problem because those functions should not be called before Interpreter['start']. I think that this code should be moved to Behavior['start'] (which is a method declared in the type but in practice it's not implemented by anything) and that Behavior['getInitialState'] has to be made pure. Accompanying tests should be introduced to make sure that we don't call side-effects within getInitialState (or within interpret(behavior).getSnapshot() as that's a more public-facing API and it's something that might lead to calling behavior.getInitialState). I've mentioned this here
5. Interpreter has two different defer methods (yes, one is on the ActorContext but that doesn't quite make it less confusing especially when both are used within some exec-related code), the first one and the second one. I think that it's best to rename one of them and that possibly it's easier to rename the second one.
6. I think that observer.done has been shoehorned on the existing concept that doesn't support this. If we have to do that, it raises the question if we are using this concept correctly. I think that it would be best to avoid such custom extensions if we plan to use to the concepts of observers, observables etc. No observable-specific code will actually be able to utilize this custom extension in a generic way and it's a bad thing. One of the main places in which Observer['done'] is being used is when the child actor gets started - we then subscribe to it (in the parent) and listen for its done callback to send the done.invoke.* even to ourselves. This is conceptually a little bit weird because the responsibility to send that event has been moved to the parent whereas the more straightforward way of doing that would be to send that even directly from the child to the parent. We don't need to use a separate "channel" for communication like this as we already have our own, standard, channel that we can use for communication. I managed to remove this in this PR. The only downside there is that onDone's implementation is a little bit funky - but that's perhaps easily solvable by just changing the callback's argument: it could be TOutput (once we add it) instead of DoneEvent. This would match what you have noted as a desired semantic for done here

[davidkpiano]

@Andarist

1. ~~Agree re: making StateMachine implement a Behavior interface. Will look into this soon.~~ Done in 39104d6
2. Discussed in Discord; we'll handle this in a separate PR with the goal of useMachine -> useBehavior and useInterpret working with any Behavior.
3. See reply - unsure what to test there.
4. This is resolved and all tests passing in #3726 ✅ The last thing to fix there is invoke(...) types.
5. ~~Will rename one of the defers, and eventually figure out a better way to handle "deferred" actions (not really deferred, just moved to the end of the queue)~~ Renamed in 951847a
6. Your PR #3720 has been merged for this 👍

We're getting close!