jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.08k stars 780 forks source link

The dispatch initializer ends in an endless loop on init when dispatching any action #1077

Open rucsi opened 2 years ago

rucsi commented 2 years ago

I'm trying to use the the dispatch initializer as it is in the example with a log effect

const middleware = (dispatch) => 
  (action, payload) => {
    // Dispatch anonymous action which triggers logging effect.
    dispatch((state) => [state, log(state)]) 

    // Afterwards, carry on normally.
    dispatch(action, payload)
  }

app({
  // ...
  dispatch: middleware
})

but it ends in an endless loop. no matter what my log effect does. even if I just do dispatch(state => state) it gets in the loop.

zaceno commented 2 years ago

@rucsi I haven't fully worked out why you get an infinite loop, but I can say what you are doing is not correct – or at least not the way middlewares are supposed to be used.

If like in your example you want to log every state change – you just log it! No need to dispatch an action with an effect for logging. That would be a kind of recursion anyway since the middleware listens in on every dispatch, and if it dispatches what it is listening to ...

I'm guessing the reason you're dispatching an action is because you want to log just state changes, and as you've realized the dispatcher can get a variety of different things as first argument. Study the code for dispatch and you will see that it is implemented in a recursive fashion so that anything that changes state will at some point dispatch(newState).

That means all you need to do is check that the action argument is neither a function nor an array – then you know it is new state.

const middleware = (dispatch) => 
  (action, payload) => {
    if (typeof action !== 'function' && !Array.isArray(action)) {
      console.log(action) //action is determined to be the next state
    }
    dispatch(action, payload)
  }
rucsi commented 2 years ago

thanks @zaceno for your response. I ended up doing something like that. I figured that dispatching from the dispatcher might not be right. the reason I did that the first place is cause I just started my project going through the documentation and it says that it should be done that way. that's the reason I raised the issue

zaceno commented 2 years ago

the documentation and it says that it should be done that way.

😱 You're right, it does say that! That is strange because I would not have thought to use middlewares that way. On the other hand, I'm still not sure why it wouldn't/shouldn't work. Maybe @icylace knows more about that particular example?

zaceno commented 2 years ago

TL;DR: The code is good. The middleware docs need to be fixed.

I've analyzed the code a bit and worked out why the infinite loop. I'm not sure I can explain it well but I'll try.

So basically the dispatch implementation is recursively "resolving" the thing that is dispatched, until it represents a new state value (and not an action). One the new state is resolved it stops recursing and instead sets the new state and schedules a rererender.

If you have an action: const AddSome = (state, add) => state + add

and you : dispatch([AddSome, 5]), dispatch will look at that and recurse: dispatch(AddSome, 5).

In the next recursion it will see that action is a function so it will recurse: dispatch(action(state, payload)). Let's say current state is 3 – that means it is recursing as dispatch(8).

Now since the value of action is 8 and not a function, hyperapp concludes that the new state is 8 and stops recursing/resolving the new state. Instead it sets the new state and schedules a DOM-update.

Ok that far – that is how the original built in dispatch is implemented. When you initialize an app with a middleware, you are essentially wrapping that, so that anything that is dispatched is first passed through your custom dispatch and from there – as you decide – passed on to the original dispatch function.

The problem is, that the recursion defined in the original dispatch is pointing to your custom dispatch which means that every step of trying to resolve an action, we are pushing a new action on the stack of things to be resolved. So we never get done resolving.

Since this recursive/resolving behavior is important and useful to be able to listen both to which actions are dispatched, and the states & effects they result in (for the purposes of testing, debugging, hooking up analytics etc etc), my conclusion is that this is not a bug – this is intended behavior.

Instead we should change that example (good find @rucsi !) for something that works, and also add to the documentation that calling dispatch inside a middleware implementation will recurse to itself. Therefore we should not dispatch any actions from within the middleware implementation.

bob-bins commented 1 year ago

@zaceno in the sample code here: https://github.com/jorgebucaran/hyperapp/issues/1077#issuecomment-997814788

state inside console.log(state) is not defined. And the official docs still has the incorrect code. I've also looked at an outdated NPM package that no longer works. Do you know if there is an example of a working logging middleware somewhere?

zaceno commented 1 year ago

@bob-bins Good catch about the sample code. Fixed it now.

I don't know about any middleware-logging packages because I normally just do what I wrote in that sample code when I need to log state (except I must have written that sample code from memory, hence the bug). There is https://github.com/mrozbarry/hyperapp-debug which does more (time travel et c), but I don't know what shape it is in currently.

zaceno commented 1 year ago

@bob-bins Actually the sample code is not really complete as it doesn't handle the case when the new state is dispatched along with effects.

I have submitted a PR to update the docs page about dispatch. In it are contained working examples of logging dispatched actions, logging state changes, and combining predefined multiple middlewares in a single app.

https://github.com/jorgebucaran/hyperapp/pull/1107

Here is the direct link to the dispatch docs that have been fixed in my open PR: https://github.com/zaceno/hyperapp/blob/7f06adb0db5343b05ea675ba1e4d1690128162e1/docs/architecture/dispatch.md