OlivierBlanvillain / monadic-html

Tiny DOM binding library for Scala.js
https://olivierblanvillain.github.io/monadic-html/examples/
MIT License
225 stars 24 forks source link

Foldp fires actions more times than needed #98

Closed antonkulaga closed 6 years ago

antonkulaga commented 6 years ago

I am trying to implement {redux, flux, outwatch}. Store pattern, I used the structure according to recommendations and I found it extremely buggy. I merge Vars for actions (like https://github.com/antonkulaga/GEOmetadb-scala/blob/6329eecc4e62d52315b57084439416c414d61eb8/web/client/src/group/research/aging/geometa/web/MainJS.scala#L29 )

val allActions: Rx[actions.Action] = toLoad merge loaded merge updateUI merge throwError
val state: Rx[states.State] = allActions.dropRepeats.foldp(states.State.empty){ case (s, a) => reducer(s, a)}.dropRepeats

and it happens that for each new action I get multiple firing of allActions. As I understand I should get all initial values (because of the way mergee works) but then, when I will update one of the Var actions it will generate me only one (state, action) update. However, what I observe is that I get a lot of updates instead, I suspect there is something wrong with the way foldp is implemented (or explained in readme).

bbarker commented 6 years ago

I might be wrong, but since state is used from more than one place, I suspect it is a sharing issue; #94 is being worked on to fix this, however, for now, you might be able to manually use sharing; you likely won't be able to do sharing after dropRepeats (iirc), but you could try doing it before -still doesn't look like that would be amazingly helpful in this case since you are running a reducer in foldp, but you could try...

antonkulaga commented 6 years ago

@bbarker I managed to find a workaround in an ugly impure way and it works as expected (despite state is shared to many places). Here is the workaround:

 val toLoad: Var[actions.ToLoad] = Var(actions.NothingToLoad)
 val loaded: Var[actions.Loaded] = Var(actions.NothingLoaded)
 val throwError: Var[actions.ExplainedError] = Var(actions.ExplainedError.empty)
 val updateUI: Var[actions.UpdateUI] = Var(actions.NotUpdateUI)

val allActions: Var[actions.Action] = Var(actions.NothingLoaded)
  protected def uglyUpdate(rxes: Rx[actions.Action]*) = {
    for(r <- rxes) r.impure.run(v=> allActions := v)
  }
  uglyUpdate(toLoad, throwError, updateUI, loaded)
  val state: Var[states.State] = Var(states.State.empty)

  //workaround to avoid foldp issues
  allActions.impure.run{ a=>
    val currentState: states.State = state.now
    val newState = reducer(currentState, a)
    if(newState != currentState) state := newState
  }
OlivierBlanvillain commented 6 years ago

@antonkulaga It's hard to give you a detailed answer without a reproducible example, but my guess is that you misunderstood what would happens at run-time when doing the following:

val state: Rx[states.State] = allActions.foldp(...)
<div>
  A. {state}
  B. {state}
  C. {state}
</div>

Here state will be interpreted 3 time, effectively creating 3 different vars for the intermediate state of foldp (for A B and C), and 3 completely disjoint event graphs. As a result, if you place a println in the body of the function used in your foldp, you will see this side effect appearing 3 times whenever a new even is fired by allActions.

While in this example it's obvious than the value of state for A B and C will be the same at any point in time, this is not true in the general setting. Which is why I don't think there is a way to change the behavior you are observing without breaking referential transparency (which you do in your workaround).

antonkulaga commented 6 years ago

@OlivierBlanvillain maybe in such case you should not recommend foldp in "{redux, flux, outwatch}.Store pattern" of readme? It is expected that most of binded values will be derived from State and that there will be multiple bindings. That means that everybody who will try to implement the pattern that way will waste a lot of times (like I did) because of mltiple firing of foldp operation.

OlivierBlanvillain commented 6 years ago

Good point, thank for pointing it out! Looking again at the Mario example (from where the pattern was extracted) the store is actually used a single time in the view, so no duplication should happens there. But just to make it clear, this duplication should only have an impact on performance as the value of every copy of your foldp should be equals, I'm I right?