Let delayed events be functions of state

[mainej]

As discussed in #7 this PR changes scheduler dispatches by making them functions of a state. This gives delayed events enough information to know what state they are resuming.

As mentioned, it is not quite working yet. There's a problem if multiple states enter the same delay at the same time.

For failing tests, see https://github.com/mainej/clj-statecharts-re-frame. (Run clj -T:build test.)

[mainej]

@lucywang000 I should have made this a draft PR. I think you have permissions to change it if you want.

[mainej]

@lucywang000 I realized that it's too hard to run tests in a separate repo. d62a059 adds a failing test to this repo which demonstrates the problem I described with many simultaneous delays. lein kaocha:

FAIL in statecharts.impl-test/test-simultaneous-delays (impl_test.cljc:1019)
Expected:
  :done
Actual:
  -:done +:running

[mainej]

The failing test is fixed in 5ee8e97. So, I think this PR is ready for consideration.

[lucywang000]

I left some comments. Overall it LGTM!

[lucywang000]

I don't like adding this new _id key for all the users of this lib. So far it's only useful for the re-frame integration, so for those who doesn't use this feature, it would be dead weight. And it's hard to change or remove it once people starts to make use of it.

What about we do not adding this key by default? The re-frame integration could always insert its own _id key during fsm init, e.g. using the _rf-path as the key. Actually it may not need to add this key at all: it could re-implement its own IScheduler type which could simply use the [_rf-path, event] as the key path.

[mainej]

Let's discuss this a bit more... I'm trying to keep this PR as narrow as possible, but there is at least one other place in the code that couples a machine to a single state. If you ever wanted to provide a Service where one machine managed several states, you'd run into this same problem again. Without something like :_id on individual states, all schedulers couple a machine to a single state. So, fixing that in the main implementation of IScheduler is a start to fixing it elsewhere.

It's not wrong that Service and re-frame couple a machine to a single state, but it is limiting. When I first read the clj-statecharts documentation it was a major insight that state and machine can and should be separate. It's a beautiful way to decouple the problem and I think statecharts should highlight and encourage that perspective.

As far as :_id vs :_rf-path goes, I was thinking the converse. If all states had an :_id, then we wouldn't need :_rf-path. We could use the :_id in the app-db path.

And as far as :_id being dead weight... yes, it adds 16 bytes to every state (or less if a user provides something other than a uuid). But it's part of ensuring you don't have to store a copy of the entire state machine alongside every state. So in terms of size, it's a win overall. And I appreciate wanting to hide data from users, but the docs make it clear that the underscored variables are reserved and shouldn't be messed with. And though a user could read the :_id, there's not much they could do with it.

So, I'm going to leave :_id and the timeout cache as is for now. Ultimately it's up to you and I'm happy to change it if you want. Let me know.

[lucywang000]

Thanks for the insights, but still I'd prefer not do add this _id field unconditionally, for these concerns:

1. by definition a state is just an value, and it should not have to have anything to do with a "unique id".

2. the unique id only makes sense when used in a scheduler, e.g. managing long-running operations like http requests, but these are not all the use cases for a state machine library. For instance I have quite a few projects that only uses the functional layer of this library, and I would not like a uuid being inserted to my state map in those cases, because it would be pure noise when I inspect the data. (the latest one of such use cases is some parser code I just write for fun here.)

but the docs make it clear that the underscored variables are reserved and shouldn't be messed with.

The doc suggests the user could read and make use of these reserved keys, like _state, _prev-state, just not to update them.

[mainej]

Fair enough. I'll take the _id out.

Without it, the new statecharts.impl-test/test-simultaneous-delays test fails. The last assertion breaks because we're back to the situation where the timeout id cache gets clobbered. I can think of several ways to address that:

1. Delete the test. Though, without it, the other changes in this PR feel pretty un-motivated.
2. Re-implement IScheduler in the test. Not my favorite. I don't like tests that dig deep into the implementation. They require a lot of knowledge to maintain and hinder refactoring.
3. Re-implement IScheduler in statecharts.delayed. The main Scheduler wouldn't need _id, but the new implementation would. (Although I'd probably make _id configurable, so users could specify _id, _rf-path, or whatever suits them.)

Number 3 is my preference for a few reasons. First, it would be more likely to stay in sync with changes to the main Scheduler. Second, I could take advantage of it in my re-frame integration. And third, it would be useful for anyone else who runs into problems managing multiple states.

Would you be OK with the third option?

Oh, and also, that parser code is pretty cool! Very nice!

[mainej]

I started to implement 3, but ended up with a 4th option, which you can see in 3ecd2f2. Instead of creating a new IScheduler implementation, it adapts the existing Scheduler to be able to manage many states, if so configured. If you'd like, I can split it into two implementations, but I think I prefer having a single code path for the scheduler internals.

[lucywang000]

Yeah the context-id function is a very clever design :P

[mainej]

I got curious about how XState would handle this. It has an example TodoMVC app. The app has a todosMachine for the todo list as a whole (mark all completed, etc.) and another todoMachine for a single todo (edit, complete, etc.) When a new todo is created, the todosMachine spawns a todoMachine actor and saves a reference to that child in its context.

What does it mean to "spawn an actor"? If you create a child machine in a very specific way, the parent can communicate with the child via its reference to the child. And via some JS magic the child can also communicate with its parent without actually having a pointer to it.

Wow! And... yikes! Hidden pointers between objects! Wouldn't that be susceptible to memory leaks too? It seems like, yes it would unless you're very careful.

I don't know what this means for clj-statecharts @lucywang000... Do you plan to implement this concept of actors? It seems more complicated than is really necessary. Clojure libraries that mimic other libraries always have the option between staying faithful to the original or being "more Clojure-like"--it's really up to you.

For what it's worth, XState also conflates a state with a machine. If you want to create many states (e.g. many todos in a todo app, or many api requests in a web app) you create many machines. I like that clj-statecharts separates those ideas a bit, but perhaps it's too big of a departure from XState.

Anyway, enough musing for a Friday evening. Thanks for your time looking at all of this so far.

[lucywang000]

Regarding the "actor" feature: I don't think I'll implement that, because it's to complex, both conceptually difficult to reason about, and hard to implement AFAICT.

Regarding feature parity with xstate: yeah this lib is inspired by xstate, and borrows its API design, but all the features that I implement is based on a practical manner, e.g. delayed transitions, parallel states, none of them are there at first, and they were added when I got some use case where I could improve my application design with these features.

So i think the main reason I don't want the actor feature is actually because so far I haven't find a use case that could justify this complexity. If one day I find that,my mind might change, I would try to add that feature even if it's difficult. Just like the implementation of the "parallel states" feature, it was harder than I expected - I had to reimplement the core logic with a more decent algorithm for that, and the impl.cljc namespace has almost half of contents replaced in this commit.

[mainej]

all the features ... were added when I got some use case

That's very practical.

had to reimplement the core logic

Yeah, state machines are deceptively simple. It only takes a few lines to implement a basic one. But when you get into the details described by Harel and SCXML, there's a lot of subtlety. Of course, with that complexity comes a lot of power. I'm really impressed you took on the challenge.

[lucywang000]

Thanks @mainej !

[mainej]

Cool! Thanks for all your input on this PR @lucywang000. I'm really happy with how the code turned out.

If you remember, when you cut the next release, will you ping me here or in clojurians (jacob.maine)?

I've been writing a re-frame integration that uses these new features. I'm happy to maintain it as a separate library, but if you'd like I can contribute it back to clj-statecharts. Let me know!

Thanks again!

[lucywang000]

Sure I'll ping you when I do the next release.

Regarding the new re-frame integration, I think it's better to maintain as a separate library.