day8 / re-frame-test

Cross platform (cljs and clj) utilities for testing re-frame applications
MIT License
109 stars 12 forks source link

:dispatch-later causes error #14

Open mike-thompson-day8 opened 6 years ago

mike-thompson-day8 commented 6 years ago

See this re-frame issue: https://github.com/Day8/re-frame/issues/441#issuecomment-353896726

Do we want to do anything about this? What would we do about it?

samroberton commented 6 years ago

The code in re-frame/interop.cljc already attempts to handle this scenario -- the chosen semantics (per the docstring for set-timeout!) are to ignore the ms value given to :dispatch-later and queue the dispatched event up immediately by using next-tick. Something about that implementation is not working correctly.

What's confusing is that the :dispatch and :dispatch-n FX handlers work without encountering this error.

... and I think that's lead me to a likely explanation of the problem, since really, the use of next-tick is the only difference between :dispatch-later, which doesn't work, and :dispatch, which does.

next-tick, in the JVM implementation, uses bound-fn to capture the state of all dynamic variables as at the time that the :dispatch-later effect is processed (this is (a) generally the correct thing to do when moving a computation onto a different thread, and (b) specifically necessarily to make sure that the dynamic variables that re-frame-test uses still work).

This use of bound-fn will thus capture the current value of re-frame.events/*handling*. In theory, that should be fine: it will capture exactly the same value that the :dispatch and :dispatch-n FX handlers will see, so you'd expect everything to work correctly, no?

But, there's an invisible coupling between the FSM in re-frame.router and the behaviour of re-frame.events/handle: when the FSM it sees an :add-event trigger, the FSM makes its decisions about whether an event is currently being handled based on its fsm-state instance field (which is not captured by bound-fn), not based on re-frame.events/*handling*. If the FSM state is :idle, then the event will immediately be processed (via run-next-tick, which will again ensure that *handling* retains its value at the time that :dispatch-later was processed), whereas if the FSM state is :scheduled or :running, the event will be adding to the queue but not immediately processed.

So I think what's happening is that in the :dispatch-later scenario, *handling* and fsm-state are effectively giving different answers to the same question ("am I currently handling an event?") because *handling* has been captured by bound-fn in next-tick.

Does that explanation make sense?

If I am correct there, then fixing it is likely to be somewhat involved. I think ideally it would involve getting rid of re-frame.events/*handling* entirely, and making dispatch-sync go through the event-queue as well (but synchronously, obviously), so that there's only on piece of state which represents the work currently being done.