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

Return the next action at the end of the middleware, as expected by redux #8

Closed
wants to merge 3 commits into from
Closed

Return the next action at the end of the middleware, as expected by redux #8

wants to merge 3 commits into from

Conversation

tmarnet
Copy link

@tmarnet tmarnet commented Aug 12, 2016

Redux is expecting middlewares to return the next action, if not some weird behavior could happen (it did in my case)

Source

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bb50774 on destinvp:add_return_middleware into dacca9a on mironov:master.

@mironov
Copy link
Owner

mironov commented Aug 12, 2016

@ThomasMarnet Hey, thanks for the heads up! I've just released the new version with simplified middleware. Could you check if it works for you please?

@tmarnet
Copy link
Author

tmarnet commented Aug 12, 2016

Works for me 👌

But, if I may, now the order of the dispatched actions is reversed. Meaning if a LOAD action is intercepted by the middleware, it will only be dispatched after the SHOW one, you know what I'm saying ?
That's in fact the reason why you had to rewrite your tests, and also the reason why I had a constant containing the next(action) at the very top of the middleware which I was returning at the end. This way, I kept the actions' order you had before.

It's not blocking, you can keep it like that, or put it back like before. The result is nearly the same and it still works in both cases.

@mironov
Copy link
Owner

mironov commented Aug 13, 2016

@ThomasMarnet Yeah, exactly. I think the way it works now is more straightforward. There's no reason to dispatch SHOW/HIDE actions after the initial action completes.

I'm closing this as of now. Please let me know if you encounter an issue with the new approach.

@mironov mironov closed this Aug 13, 2016
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

Successfully merging this pull request may close these issues.

None yet

4 participants