FormidableLabs / freactal

Clean and robust state management for React and React-like libs.
MIT License
1.65k stars 46 forks source link

Change both states on different parent states #35

Closed yjcxy12 closed 7 years ago

yjcxy12 commented 7 years ago

Just had read through the docs and can't wait to try it. However I have came up with questions when reading sections on changeBothStates effect.

In the doc the effect is defined in Parent state, which it accesses an effect on GrandParent. This seem like a design that asks for trouble as the Parent state should be aware of the information in GrandParent. This causes questions on separation of concerns. What if changeGrandParentState has been removed or changed to multiple functions? I would have to change changeBothStates in Parent effects and all states that accesses changeGrandParentState effect. What is the thoughts behind the merge all effects into one global effects design?

Another concern is that instead of Promise.then(), in some cases Promise.all() may be preferred, in Redux world it would be one action triggers changes in multiple reducers. Would this work for these situations?

changeBothStates: (effects, value) =>
      Promise.all([
            effects.changeGrandParentState(value),
            effects.changeParentState(value),
      ])
divmain commented 7 years ago

It may be worth updating the docs with some warnings. Fundamentally, I agree: when one state container accesses the effects of a parent state container, it can muddle the separation of concerns somewhat. For that reason, it should be done sparingly and judiciously.

However, the functionality is important to make available for the simple reason that, for some applications (that have a global spinner, for example), everything would have to be at the root state container in order to avoid reaching across containers.

The Promise.all snippet seams a reasonable way to organize your code, if it seems more readable to you - I don't intend freactal to be prescriptive in this area. However, I should emphasize that the snippet as written would cause an error, since the effect doesn't resolve to a state-transformation function. For that reason, it might be better to pull changeBothStates out into a simple helper function that accepts effects and value from its caller.