salsita / redux-side-effects

Redux toolset for keeping all the side effects inside your reducers while maintaining their purity.
MIT License
182 stars 9 forks source link

A few ideas #8

Closed calebmer closed 8 years ago

calebmer commented 8 years ago

I really like the direction this module takes to side effects in redux, and I've been exploring its usages in an isomorphic progressively enhanced web app. However, there are a couple of things which I have questions on/would like to see changed. I am willing to write a PR for all these things if you would like.

  1. Rename createEffectCapableStore to applySideEffects or something like that (this is just a preference thing so it is more inline with the Redux applyMiddleware).
  2. Whatever is yielded is dispatched. So if a user yields an action object, that will be dispatched. If the user yields a function, that will be dispatched. This would require the use of redux-thunk middleware to accomplish the current functionality, but I think that's desired for most redux developers.
  3. Defer side effect execution to after the reducer using setImmediate instead of executing the effects on the next dispatch. I'm not sure why effects are executed in the next dispatch currently. This change would eliminate the need for AppStateWithEffects and for wrapping the dispatch function. This change would also have the benefit of making the module's surface area less.
  4. Have a hook/promise which executes/resolves when all side effects have completed. I'm not exactly sure how this one would work, but because I mainly use this in a server rendering data prefetching context I need to know when all side effects have been resolved. Maybe this could be accomplished (with the implementation of suggestion 2) with an array which collects all the return values of dispatched side effects which I could use Promise.all(…) on later.

I really love this module, I love the approach it's taking, I live the literature surrounding it, and I am really excited for what it could mean in the progressive enhancement community. These are just a couple suggestions/challenges/questions (call it what you want ☺️) I had while exploring the source code.

tomkis commented 8 years ago

Thank you!

  1. Yes we need to think about the naming but its breaking change so we have to figure out proper naming before releasing 1.0.0. I am not sure about applySideEffects though. applyMiddleware is a good name even for store enhancer because it actually takes middleware list as an argument on the other hand applySideEffects does not take any arguments (hypothetically it does not apply anything) therefore I am a bit worried about some confusion. I checked all the store enhancers listed in redux ecosystem page yet I didn't find any pattern in naming. But many people simply use default export for the enhancer, maybe this is the way to go...
  2. Important thing to realize is that you should never dispatch action within reducer because this leads to cascading updates and unpredictable dataflow, therefore yielding actions to be dispatched directly should definitely be banned, in fact I actually decided to ban yielding anything except thunk
  3. I guess you are probably talking about this - it's actually in the same dispatch cycle. Anyway, deferring the actual execution, I don't see how this can help us to avoid using AppStateWithEffects, can you please extend?
  4. Yes, we could chain all the results from reducer thunks and promisify them (unless they are promises already) something like redux-promise is doing so the actual dispatch chain would return a promise (no matter whether any promise was provided).
calebmer commented 8 years ago
  1. We could provide both a default export and a named one. Just the current name is just a little confusing 😊
  2. That's an interesting idea, chaining all the promises. That wasn't the direction I was thinking. My thoughts were something along the lines of exposing the return values of the side effects in some way.

As for 2 and 3 I'll put together some code as a discussion peice.

tomkis commented 8 years ago

Closing in favor of next