Change side effect dispatch model

[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:

• Add defer/clearDefer utilities (just a setTimeout(callback, 0) abstraction)
• Remove unused code
• Tests
• Readme

[tomkis]

As explained:

1. It should definitely be banned to dispatch an action here, only thunks should be allowed otherwise we would allow user to cascade actions.
2. We can't afford reliance on user's redux-thunk as peer dependency

[tomkis]

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]

There were a couple goals of this PR: reduce surface area (only wrap a single function), remove AppStateWithEffects, allow the dispatch of anything (🙃), and expose the dispatch results.

I think just because we think a user shouldn't do something, that doesn't mean we should enforce it. There might be some who argue side effects in a reducer should never be done, but because it can be done we can experiment with different styles. This applies to two things.

First is the anything-dispatching. I don't think I completely understand the technical limitation keeping us from allowing it. Technically all actions (whether thunked or not) are dispatched, and what is the real benefit of yield dispatch => dispatch('FOO') over yield 'FOO'?

I mainly want this because I have some custom middleware which responds to promises, so writing yield Promise.resolve('FOO') is a nice syntactic thing.

Second, I completely agree that effects shouldn't have effects, but I don't think we should enforce that. Once an action is dispatched it would be weird to (externally or internally) keep some metadata on it disallowing further effect propagation. Once an effect is dispatched it should behave like any other action. In some systems this also might be useful.

Finally, I'll have to see your promise chain implementation. My initial reaction is that I'm not sure I like it. Adding promises kinda removes the purity of the implementation. I am curious about the "+ middleware" part though.

[tomkis]

@calebmer

There were a couple goals of this PR: reduce surface area (only wrap a single function), remove AppStateWithEffects, allow the dispatch of anything (🙃), and expose the dispatch results.

Excellent! I agree with:

• reduce surface area - though I am not sure whether we manage to wrap just single function
• remove AppStateWithEffects
• expose dispatch results

I think just because we think a user shouldn't do something, that doesn't mean we should enforce it. There might be some who argue side effects in a reducer should never be done, but because it can be done we can experiment with different styles. This applies to two things.

Yes and No. Just keep in mind that even when using redux-side-effects we are not breaking the premise of pure reducers with no side effects. We are effectively getting rid of this very important condition. So even in original createStore in Redux this is something banned.

First is the anything-dispatching. I don't think I completely understand the technical limitation keeping us from allowing it. Technically all actions (whether thunked or not) are dispatched, and what is the real benefit of yield dispatch => dispatch('FOO') over yield 'FOO'?

All I wanted to say is that either yield dispatch => dispatch('FOO') or yield 'FOO' is wrong because this is something called action cascading which has always been banned and we should not break it. First time I was working with Flux It didn't took me a long till I realized that action chaining is evil and everything becomes quite tangled. I try to think about actions as Events now and everything is much more easier to reason about. I believe the well known exception "Cannot dispatch in the middle of dispatch" helped dozens of people with proper Flux architecture.

I mainly want this because I have some custom middleware which responds to promises, so writing yield Promise.resolve('FOO') is a nice syntactic thing.

Agreed, I might have not expressed myself clear enough. It should not be allowed to dispatch sync action in effect. Promise is by nature async therefore it makes sense to accept either thunk or Promise

Second, I completely agree that effects shouldn't have effects, but I don't think we should enforce that. Once an action is dispatched it would be weird to (externally or internally) keep some metadata on it disallowing further effect propagation. Once an effect is dispatched it should behave like any other action. In some systems this also might be useful.

To advocate this it would require probably whole blog post, for starters, it may be useful to at least log some warning that it's not ideal to cascade async actions.

Finally, I'll have to see your promise chain implementation. My initial reaction is that I'm not sure I like it. Adding promises kinda removes the purity of the implementation. I am curious about the "+ middleware" part though.

Here it is

[calebmer]

Either yield dispatch => dispatch('FOO') or yield 'FOO' is wrong because this is something called action cascading which has always been banned and we should not break it.

Ok, I get it now. However, I still believe in dispatching whatever is yielded for the middleware reason I stated above. A warning/error when yielding a plain object is totally understandable though.

---

I think development warnings are a good idea to solve some of the pattern breaking issues (plain object dispatch & effect effects) but I don't think we should limit technical capabilities.

[calebmer]

Ok, the code is ready to merge except for README information and I haven't changed some of the things you mentioned as I'm a bit confused on what you want, I'll ask for clarification where necessary. Also changed is that if you take a look at 3d94095, I think that's a much better solution then what I had before or the proposed change in #10. It's a very clean API and gives the user control of the deference mechanism.

Points of contention that are still included in this PR are:

• Dispatch of whatever gets yielded (requires redux-thunk for current functionality).
• Side effects can't have effects.

I'm willing to put error messages in development for both of these, however I think it best if we leave the capability to do these things even if they aren't best practice (for extensibility purposes).

Finally, to tie a nice bow on this PR do you want to rename createEffectCapableStore here or in another PR? My vote is for applyEffectCapability or applyEffectCapabilities. I think making it a function similar to applyMiddleware in allowing it to take a config option could be helpful if that helps to relate the two names (one config option could be never deferring the dispatch of effects forcing the developer to call dispatchEffects whenever they wanted the effects to be run).

[calebmer]

Ok, added the warnings for anti patterns for discussion.

[tomkis]

Thanks @calebmer, will merge the code in temp branch. There are few things I want to polish. For example I would like to get rid of Task class completely.

[calebmer]

Ok, sounds great 😊

[tomkis]

Hello @calebmer,

sorry it took me so long but I would like to revive the discussion. We've had some issues trying to solve how everything fits into a larger application and we would really like to see this PR merged, however I'd rather avoid over-engineering stuff. There have also been many changes in the overall architecture decisions which come up from redux-elm

I have incorporated some of your ideas and tried to implement something simple and here's the starting point 875ddc6

Could you please have a look whether this kind of change makes sense for you? We'd need somehow expose dispatchEffects function to exactly match interface that you'v proposed but I have an idea.

[calebmer]

Hey, long time no see 😊 I've actually come full circle too, especially after seeing this proposal.

I've moved on to another project since this PR, but I'm looking for a chance to try it again soon. That said, it took me a bit to get back in the mindset. The code in 875ddc6 looks great, much cleaner than what this PR ended up becoming.

As for dispatchEffects, I'm going to leave a comment on the commit to throw my 2 cents in on how I'd implement dispatchEffects.

So at this point, should we start thinking about closing this PR in favor of the next branch?

[calebmer]

If there's any other way I could help, please let me know 😊

[tomkis]

Continuations with Effect Handlers is a nice idea, yet I am afraid that major drawback of the solution is that propagation of side effects is not explicit. It works like try/catch you can basically throw an error deeper in the hierarchy and exception automatically propagates up until it reaches first catch block, which is not explicit and is a bit difficult to reason about.

Thanks for your help @calebmer closing in favor of next