tomkis
commented
8 years ago The PR is much appreciated!
I left few comments, these are just cosmetic things, mostly related to code style and let's discuss few conceptual problems/ideas here in the discussion.
Let me first ask a question, is the "all effects have been dispatched" hook the only purpose of this PR?
I definitely like the idea of providing the dispatch directly to enhanced reducer so that we can get rid of AppStateWithEffects, I must have been drunk while I was thinking about it. Looks like my head was too tied with the previous implementation also the idea about thinking of iterable with last element acting as app state is great. These two things should definitely get merged.
Now few problems:
Only thunks are allowed as side effects
I explained this in the comment already, we should not allow user to dispatch synchronous action. Any action dispatched via effect should be async and this means that only thunks should be acceptable. I would rather avoid dependency on redux-thunk therefore we would need to have this implementation here.
Now you might protest because I am saying that only async thunks are accepted yet we are not deferring the action execution in the current implementation but you are doing that in your PR meaning that it's better to do it your way. However, I believe that we should not explicitly defer the action execution.
Dispatched actions within effect should be naturally asynchronous otherwise it's action chaining which is wrong, user's responsibility is to met this condition, our responsibility is to verify that they are not doing that.
Action dispatched via side effect should not allow yielding another effects which dispatch another actions
In the current implementation we are ensuring the exact opposite. Few days ago I realized that this approach is actually wrong. I always kept advocating the idea that as long as the action is dispatched asynchronously it's not cascading event and it's still a valid point but the argument about single interaction entry point is much more stronger.
Therefore this is not ideal:
function* reducer(appState, action) {
if (action === 'FOO') {
yield dispatch => asyncWork().then(() => dispatch('BAR'));
} else if (action === 'BAR') {
yield dispatch => asyncWork().then(() => dispatch('BAZ'));
}
return appState;
}We should instead enforce doing this:
function* reducer(appState, action) {
if (action === 'FOO') {
yield dispatch => {
asyncWork()
.then(() => dispatch('BAR'))
.then(asyncWork)
.then(() => dispatch('BAZ'));
}
}
return appState;
}We are obviously giving up some flexibility but this approach will make the code much more easier to reason about. Because of this invariant we'll proably have to keep the dispatch method wrapped anyway.
We can use Promise chain + middlewares
If the answer for my question is yes, then I believe we can implement it with less intrusive implementation which will use promise chain and middleware. Bottom line: we can definitely use some parts of your code but let me first do some proof of concept with less intrusive implementation.
calebmer
I didn't have much time today to flesh out my ideas, but here is a rough first draft for some of my ideas in #8. I decided to push now (with all tests failing ☺️) as the changes are pretty much how I envisioned them, so we can use them as a discussion point. What's left to do is some admin stuff:
defer/clearDeferutilities (just asetTimeout(callback, 0)abstraction)