Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rucsi opened this issue Dec 17, 2021 · 7 comments

Comments

@rucsi
Copy link

rucsi commented Dec 17, 2021

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
Copy link
Contributor

zaceno commented Dec 20, 2021

@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
Copy link
Author

rucsi commented Dec 20, 2021

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
Copy link
Contributor

zaceno commented Dec 20, 2021

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
Copy link
Contributor

zaceno commented Dec 20, 2021

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
Copy link

bob-bins commented Feb 8, 2023

@zaceno in the sample code here: #1077 (comment)

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
Copy link
Contributor

zaceno commented Feb 8, 2023

@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
Copy link
Contributor

zaceno commented Feb 8, 2023

@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.

#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants