Closed mainej closed 3 years ago
@lucywang000 I should have made this a draft PR. I think you have permissions to change it if you want.
@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
The failing test is fixed in 5ee8e97. So, I think this PR is ready for consideration.
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.
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.
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.
Thanks @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!
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.
As discussed in https://github.com/lucywang000/clj-statecharts/pull/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
.)~