A concern on combineReducer, when an action is related to multiple reducers #601

Closed
jiyinyiyong opened this Issue Aug 21, 2015 · 46 comments

Comments

@jiyinyiyong

I'm working on a chat app built with React.js and Flux. As I read the docs of Redux the combineReducer function appears strange to me. For example:

There are three stores called messageStore, unreadStore, threadStore. And there's and action called NEW_MESSAGE. All three stores will update on new messages. In Redux, the stores are reducers combined with combineReducer like:

message = (state=[], action) ->
  switch action.type
    when NEW_MESSAGE
      state # new state
unread = (state=[], action) ->
  switch action.type
    when NEW_MESSAGE
      state # new state
thread = (state=[], action) ->
  switch action.type
    when NEW_MESSAGE
      state # new state

combineReducer {message, unread, thread}

It appears to me the in such cases, splitting reducers is not making my life easier. However a giant store built with immutable-js maybe a better solution for this. Like:

initialState = Immutable.fromJS
  message: []
  unread: []
  thread: []

allStore = (store=initialState, action) ->
  switch action.type
    when NEW_MESSAGE
        initialState
        .updateIn ['message'], (messages) -> messages # updated
        .updateIn ['urnead'], (urneads) -> unreads # updated
        .updateIn ['thread'], (threads) -> threads # updated

@gaearon gaearon added the question label Aug 21, 2015

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 21, 2015

Collaborator

Can you elaborate why you think combineReducers isn't helpful in this case? Indeed, for database-like state, you often want a single reducer (and then other reducers for UI state, for example, selected item, etc).

See also:

https://github.com/rackt/redux/tree/master/examples/real-world/reducers
https://github.com/rackt/redux/blob/master/examples/async/reducers

for inspiration.

Collaborator

gaearon commented Aug 21, 2015

Can you elaborate why you think combineReducers isn't helpful in this case? Indeed, for database-like state, you often want a single reducer (and then other reducers for UI state, for example, selected item, etc).

See also:

https://github.com/rackt/redux/tree/master/examples/real-world/reducers
https://github.com/rackt/redux/blob/master/examples/async/reducers

for inspiration.

@jiyinyiyong

This comment has been minimized.

Show comment
Hide comment
@jiyinyiyong

jiyinyiyong Aug 21, 2015

The first exmple you pointed about is similar to my case, requestType is handled in both reducers.
https://github.com/rackt/redux/blob/master/examples/real-world/reducers/paginate.js#L28

Splitting reducers makes it easier for maintaining when two reducers are seperated from earch others. But when they are handing to the same actions, it leads to:

  • for such an action, I have to maintain it in several reducers(or files after my app grows). In our current codebase we already saw 5 stores handling a same action, it's a bit hard to maintain.
  • in some cases, one reducer may depend on data from another(like moving a message from unread to message, or even promote a message as a new thread from message to thread). So I may need to get data from another reducer now.

I don't see a nice solution for these two problems. Actually I'm not totally sure about these two problems, they are like tradeoffs.

The first exmple you pointed about is similar to my case, requestType is handled in both reducers.
https://github.com/rackt/redux/blob/master/examples/real-world/reducers/paginate.js#L28

Splitting reducers makes it easier for maintaining when two reducers are seperated from earch others. But when they are handing to the same actions, it leads to:

  • for such an action, I have to maintain it in several reducers(or files after my app grows). In our current codebase we already saw 5 stores handling a same action, it's a bit hard to maintain.
  • in some cases, one reducer may depend on data from another(like moving a message from unread to message, or even promote a message as a new thread from message to thread). So I may need to get data from another reducer now.

I don't see a nice solution for these two problems. Actually I'm not totally sure about these two problems, they are like tradeoffs.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 21, 2015

Collaborator

for such an action, I have to maintain it in several reducers(or files after my app grows). In our current codebase we already saw 5 stores handling a same action, it's a bit hard to maintain.

Whether this is good or bad depends on your app. In my experience having many stores (or Redux reducers) handling the same action is actually very convenient because it divides responsibilities, and also lets people work on feature branches without colliding with the rest of the team. In my experience it's easier to maintain unrelated mutations separately than one giant mutation.

But you're right there are cases where this doesn't work too well. I'd say they are often symptoms of a suboptimal state model. For example,

in some cases, one reducer may depend on data from another(like moving a message from unread to message, or even promote a message as a new thread from message to thread)

is a symptom of a problem. If you have to move stuff inside your state a lot, maybe the state shape needs to be more normalized. Instead of { readMessages, unreadMessages } you can have { messagesById, threadsById, messageIdsByThreadIds }. Message read? Fine, toggle state.messagesById[id].isRead. Want to read messages for a thread? Grab state.threadsById[threadId].messageIds, then messageIds.map(id => state.messagesById[id]).

When data is normalized, you don't really need to copy items from one array to another array. Most of the times you'd be flipping flags or adding/removing/associating/disassociating IDs.

Collaborator

gaearon commented Aug 21, 2015

for such an action, I have to maintain it in several reducers(or files after my app grows). In our current codebase we already saw 5 stores handling a same action, it's a bit hard to maintain.

Whether this is good or bad depends on your app. In my experience having many stores (or Redux reducers) handling the same action is actually very convenient because it divides responsibilities, and also lets people work on feature branches without colliding with the rest of the team. In my experience it's easier to maintain unrelated mutations separately than one giant mutation.

But you're right there are cases where this doesn't work too well. I'd say they are often symptoms of a suboptimal state model. For example,

in some cases, one reducer may depend on data from another(like moving a message from unread to message, or even promote a message as a new thread from message to thread)

is a symptom of a problem. If you have to move stuff inside your state a lot, maybe the state shape needs to be more normalized. Instead of { readMessages, unreadMessages } you can have { messagesById, threadsById, messageIdsByThreadIds }. Message read? Fine, toggle state.messagesById[id].isRead. Want to read messages for a thread? Grab state.threadsById[threadId].messageIds, then messageIds.map(id => state.messagesById[id]).

When data is normalized, you don't really need to copy items from one array to another array. Most of the times you'd be flipping flags or adding/removing/associating/disassociating IDs.

@gaearon gaearon closed this Aug 21, 2015

@jiyinyiyong

This comment has been minimized.

Show comment
Hide comment
@jiyinyiyong

jiyinyiyong Aug 22, 2015

I'd like to try that.

I'd like to try that.

@jiyinyiyong

This comment has been minimized.

Show comment
Hide comment
@jiyinyiyong

jiyinyiyong Aug 23, 2015

I just wrote a demo page to try redux. I think combineReducer brought quite some complexity to me. And all the examples in the repo are using combineReducer, is there still chance I can use a single immutable data object as the model, rather than an JavaScript object that contains my data?

I just wrote a demo page to try redux. I think combineReducer brought quite some complexity to me. And all the examples in the repo are using combineReducer, is there still chance I can use a single immutable data object as the model, rather than an JavaScript object that contains my data?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 23, 2015

Collaborator

@jiyinyiyong I'm really not sure what complexity you are talking about, but feel free to not use combineReducers. It's just a helper. Check out a similar helper that uses Immutable instead. Or write your own of course.

Collaborator

gaearon commented Aug 23, 2015

@jiyinyiyong I'm really not sure what complexity you are talking about, but feel free to not use combineReducers. It's just a helper. Check out a similar helper that uses Immutable instead. Or write your own of course.

@jokeyrhyme

This comment has been minimized.

Show comment
Hide comment
@jokeyrhyme

jokeyrhyme Aug 29, 2015

I wrote my own combineReducers() helper mostly to support Immutable.js, but it can take additional reducers that treated like root reducers:
https://github.com/jokeyrhyme/wow-healer-bootcamp/blob/master/utils/combineReducers.js

The first argument matches the shape of your state and passes the top-level sub-states to the matching sub-reducers that you provide. This is more or less the same as the official version.

However, it is optional to add zero or more additional arguments, these are treated like other root reducers. These reducers are passed the complete state. This allows actions to be dispatched in ways that require more access than the recommended sub-reducer pattern.

It's obviously not a great idea to abuse this, but I've found the odd case where it's nice to have multiple sub-reducers as well as multiple root reducers.

Perhaps instead of adding multiple root reducers, a middle step could be using the additional arguments to define sub-reducers that need broader access (still less than total access). For example:

function a (state, action) { /* only sees within a */ return state; }
function b (state, action) { /* only sees within b */ return state; }
function c (state, action) { /* only sees within c */ return state; }

function ab (state, action) { /* partial state containing a and b */ return state; }
function bc (state, action) { /* partial state containing b and c */ return state; }

let rootReducer = combineReducers(
  { a, b, c },
  [ 'a', 'b', ab ],
  [ 'b', 'c', bc ]
);

Is it worth submitting a PR for partial reducers and additional root reducers? Is this a horrible idea, or is this something that could be useful to enough other people?

I wrote my own combineReducers() helper mostly to support Immutable.js, but it can take additional reducers that treated like root reducers:
https://github.com/jokeyrhyme/wow-healer-bootcamp/blob/master/utils/combineReducers.js

The first argument matches the shape of your state and passes the top-level sub-states to the matching sub-reducers that you provide. This is more or less the same as the official version.

However, it is optional to add zero or more additional arguments, these are treated like other root reducers. These reducers are passed the complete state. This allows actions to be dispatched in ways that require more access than the recommended sub-reducer pattern.

It's obviously not a great idea to abuse this, but I've found the odd case where it's nice to have multiple sub-reducers as well as multiple root reducers.

Perhaps instead of adding multiple root reducers, a middle step could be using the additional arguments to define sub-reducers that need broader access (still less than total access). For example:

function a (state, action) { /* only sees within a */ return state; }
function b (state, action) { /* only sees within b */ return state; }
function c (state, action) { /* only sees within c */ return state; }

function ab (state, action) { /* partial state containing a and b */ return state; }
function bc (state, action) { /* partial state containing b and c */ return state; }

let rootReducer = combineReducers(
  { a, b, c },
  [ 'a', 'b', ab ],
  [ 'b', 'c', bc ]
);

Is it worth submitting a PR for partial reducers and additional root reducers? Is this a horrible idea, or is this something that could be useful to enough other people?

@jiyinyiyong

This comment has been minimized.

Show comment
Hide comment
@jiyinyiyong

jiyinyiyong Aug 30, 2015

I feel more like to learn how halogen and elm is dealing with these problems. JavaScript is mixed of several progammig diagrams and leading us to many ways. I'm now confused which is the right one of FRP and unidirectional data flow.

I feel more like to learn how halogen and elm is dealing with these problems. JavaScript is mixed of several progammig diagrams and leading us to many ways. I'm now confused which is the right one of FRP and unidirectional data flow.

@rhys-vdw

This comment has been minimized.

Show comment
Hide comment
@rhys-vdw

rhys-vdw Jan 25, 2016

I have found this to be a point of confusion for me also. Just posting my use case/solution here.

I have three subreducers that handle an array of three different resource types (clients, projects, jobs). When I delete a client (REMOVE_CLIENT), all related projects and jobs must be deleted. Jobs are related to clients through a project, so the jobs reducer must have access to the projects to do the join logic. The jobs reducer must get a list of all project IDs that belongs to the client being deleted, and then remove any matching job from the store.

I got around this issue with the following middleware:

export default store => next => action => {
  next({ ...action, getState: store.getState });
}

So that every action has access to the root store via action.getState().

I have found this to be a point of confusion for me also. Just posting my use case/solution here.

I have three subreducers that handle an array of three different resource types (clients, projects, jobs). When I delete a client (REMOVE_CLIENT), all related projects and jobs must be deleted. Jobs are related to clients through a project, so the jobs reducer must have access to the projects to do the join logic. The jobs reducer must get a list of all project IDs that belongs to the client being deleted, and then remove any matching job from the store.

I got around this issue with the following middleware:

export default store => next => action => {
  next({ ...action, getState: store.getState });
}

So that every action has access to the root store via action.getState().

@pollen8

This comment has been minimized.

Show comment
Hide comment
@pollen8

pollen8 Feb 10, 2016

@rhys-vdw thanks for that! A really helpful middleware :)

pollen8 commented Feb 10, 2016

@rhys-vdw thanks for that! A really helpful middleware :)

@raphaelokon

This comment has been minimized.

Show comment
Hide comment
@raphaelokon

raphaelokon Feb 14, 2016

@rhys-vdw Thanks for this – seems like a nice solution. How do you handle edge cases/referential integrity, e.g. when an entity is shared by two (or more) other entities or pointing to a non-existing record during deleting. Just keen to hear your thoughts on this.
@gaearon Is there a documented way in Redux how to solve this kind of referential integrity problem of normalised entities?

@rhys-vdw Thanks for this – seems like a nice solution. How do you handle edge cases/referential integrity, e.g. when an entity is shared by two (or more) other entities or pointing to a non-existing record during deleting. Just keen to hear your thoughts on this.
@gaearon Is there a documented way in Redux how to solve this kind of referential integrity problem of normalised entities?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 14, 2016

Collaborator

There is no specific way. I would imagine that one can automate this by defining a schema and generating reducers based on this schema. They would then “know” how to collect “garbage” when things are deleted. However this is all very hand wavy and requires implementation effort :-). Personally I don’t think deleting happens often enough to warrant real cleanup. I would usually flip a status field on the model and make sure to not display deleted items when selecting them. Or, of course, you can refresh on delete if it is not a common operation.

Collaborator

gaearon commented Feb 14, 2016

There is no specific way. I would imagine that one can automate this by defining a schema and generating reducers based on this schema. They would then “know” how to collect “garbage” when things are deleted. However this is all very hand wavy and requires implementation effort :-). Personally I don’t think deleting happens often enough to warrant real cleanup. I would usually flip a status field on the model and make sure to not display deleted items when selecting them. Or, of course, you can refresh on delete if it is not a common operation.

@raphaelokon

This comment has been minimized.

Show comment
Hide comment
@raphaelokon

raphaelokon Feb 14, 2016

@gaearon Cheers for the super fast reply. Yeah … I think one has the same effort in dealing with referential integrity when using references with document-based DBs like Mongo. I think it is worth either to have a pure state without any garbage entities floating around. Not sure if those ghost entries could ever trip you up especially when you have a lot of CRUD ops on your state.

However I also think it would be ace if Redux had support for both – normalised and embedded entities. And one could pick a strategy that fits for purpose. But I guess for embedded entities one must use nested reducers which is an anti-pattern according the Redux docs.

@gaearon Cheers for the super fast reply. Yeah … I think one has the same effort in dealing with referential integrity when using references with document-based DBs like Mongo. I think it is worth either to have a pure state without any garbage entities floating around. Not sure if those ghost entries could ever trip you up especially when you have a lot of CRUD ops on your state.

However I also think it would be ace if Redux had support for both – normalised and embedded entities. And one could pick a strategy that fits for purpose. But I guess for embedded entities one must use nested reducers which is an anti-pattern according the Redux docs.

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Mar 14, 2016

I found it difficult to access the rest of store state in a reducer. It makes nested combineReducers really difficult to use in some cases. It would be good if there are methods to access the rest of the state like .parent() or .root() or equivalent.

I found it difficult to access the rest of store state in a reducer. It makes nested combineReducers really difficult to use in some cases. It would be good if there are methods to access the rest of the state like .parent() or .root() or equivalent.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 14, 2016

Collaborator

Accessing the state of other reducers in a reducer is most often an anti-pattern.
It can be usually solved by:

  • Removing that logic from reducer and moving it to a selector;
  • Passing additional information into the action;
  • Letting the view code perform two actions.
Collaborator

gaearon commented Mar 14, 2016

Accessing the state of other reducers in a reducer is most often an anti-pattern.
It can be usually solved by:

  • Removing that logic from reducer and moving it to a selector;
  • Passing additional information into the action;
  • Letting the view code perform two actions.
@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Mar 14, 2016

Thanks! I agree with that. I just found it became more complex after trying to pass the root state into reducers :-)

Thanks! I agree with that. I just found it became more complex after trying to pass the root state into reducers :-)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 14, 2016

Collaborator

Yeah, the problem with passing the root state is that reducers become coupled to each other’s state shape which complicates any refactoring or change in the state structure.

Collaborator

gaearon commented Mar 14, 2016

Yeah, the problem with passing the root state is that reducers become coupled to each other’s state shape which complicates any refactoring or change in the state structure.

@raphaelokon

This comment has been minimized.

Show comment
Hide comment
@raphaelokon

raphaelokon Apr 2, 2016

Okay, I see clearly the benefits of having a normalised state and treat the redux part of my state kind like a DB.

I am still thinking if @gaearon's (flagging entities for deletion) or @rhys-vdw's approach is better (resolving the references and remove related references in place).

Any ideas/real world experiences?

Okay, I see clearly the benefits of having a normalised state and treat the redux part of my state kind like a DB.

I am still thinking if @gaearon's (flagging entities for deletion) or @rhys-vdw's approach is better (resolving the references and remove related references in place).

Any ideas/real world experiences?

@kurtharriger

This comment has been minimized.

Show comment
Hide comment
@kurtharriger

kurtharriger Apr 2, 2016

If redux was more like elm and the state was not normalized then decoupling reducers from state shape makes sence. However since the state is normalized it seems reducers often need access to other parts of the app state.

Reducers are already transitively coupled to the application since reducers depend on actions. You cannot trivially reuse reducers in another redux application.

Working aroung the issue by pushing state access into actions and using middleware seems like a much uglier form of coupling then allowing reducers to access root state directly. Now actions that ofterwise do not require it are coupled to state shape too and there are more actions than reducers and more places to change.

Passing state into view into actions is perhaps better insulates in state shape changes, but if additional state is needed as features are added just as many actions need to be updated.

IMHO allowing reducers access to the root state would result in less overall coupling than the current workarounds.

If redux was more like elm and the state was not normalized then decoupling reducers from state shape makes sence. However since the state is normalized it seems reducers often need access to other parts of the app state.

Reducers are already transitively coupled to the application since reducers depend on actions. You cannot trivially reuse reducers in another redux application.

Working aroung the issue by pushing state access into actions and using middleware seems like a much uglier form of coupling then allowing reducers to access root state directly. Now actions that ofterwise do not require it are coupled to state shape too and there are more actions than reducers and more places to change.

Passing state into view into actions is perhaps better insulates in state shape changes, but if additional state is needed as features are added just as many actions need to be updated.

IMHO allowing reducers access to the root state would result in less overall coupling than the current workarounds.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Apr 2, 2016

Contributor

@kurtharriger : note that while the common recommended pattern is to structure Redux state by domain, and use combineReducers to manage each domain separately, that is just a recommendation. It is entirely possible to write your own top-level reducer that manages things differently, including passing the entire state to specific sub-reducer functions. Ultimately, you really only have _one_ reducer function, and how you break it down internally is entirely up to you. combineReducers is just a useful utility for a common use case.

The Redux FAQ answer on splitting business logic also has some good discussion on this topic.

Contributor

markerikson commented Apr 2, 2016

@kurtharriger : note that while the common recommended pattern is to structure Redux state by domain, and use combineReducers to manage each domain separately, that is just a recommendation. It is entirely possible to write your own top-level reducer that manages things differently, including passing the entire state to specific sub-reducer functions. Ultimately, you really only have _one_ reducer function, and how you break it down internally is entirely up to you. combineReducers is just a useful utility for a common use case.

The Redux FAQ answer on splitting business logic also has some good discussion on this topic.

@kurtharriger

This comment has been minimized.

Show comment
Hide comment
@kurtharriger

kurtharriger Apr 2, 2016

Yes another option would be to just use a single reducer in one giant function and/or write you're own combineReducer function rather than use the built in one.
Having a built in combineReducers method that adds an additional root state argument could be useful, but PRs that have attempted add this functionality have been closed as an anti-pattern. I just wanted to argue the point the alternatives proposed IMHO result in much more overall coupling and maybe this shouldn't be considered an anti-pattern. Also if root state was added as an additional argument its not like you are forced to use is so the coupling still an opt-in.

Yes another option would be to just use a single reducer in one giant function and/or write you're own combineReducer function rather than use the built in one.
Having a built in combineReducers method that adds an additional root state argument could be useful, but PRs that have attempted add this functionality have been closed as an anti-pattern. I just wanted to argue the point the alternatives proposed IMHO result in much more overall coupling and maybe this shouldn't be considered an anti-pattern. Also if root state was added as an additional argument its not like you are forced to use is so the coupling still an opt-in.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 13, 2016

Collaborator

We want this coupling to be explicit. It is explicit when you do it manually and it's literally N lines of code where N is how many reducers you have. Just don't use combineReducers and write that parent reducer by hand.

Collaborator

gaearon commented Apr 13, 2016

We want this coupling to be explicit. It is explicit when you do it manually and it's literally N lines of code where N is how many reducers you have. Just don't use combineReducers and write that parent reducer by hand.

@jwhiting

This comment has been minimized.

Show comment
Hide comment
@jwhiting

jwhiting Apr 17, 2016

I'm new to redux/react but would humbly ask this: if composed reducers accessing each other's state is an anti-pattern because it adds coupling among disparate areas of the state shape, I would argue this coupling is already taking place in mapStateToProps() of connect(), since that function provides the root state and components pull from everywhere/anywhere to get their props.

If state subtrees are to be encapsulated, components should have to access state through accessors that hide those subtrees, so that state shape internal to those scopes can be managed without refactoring.

As I learn redux, I am struggling with what I envision to be a long term problem with my app, which is tight coupling of state shape across the app - not by reducers but by mapStateToProps - such as a comment sidebar that has to know the auth user's name (each reduced by a comment reducer vs auth reducer, for instance.) I am experimenting with wrapping all state access in accessor methods that do this kind of encapsulation, even in mapStateToProps, but feel like I might be barking up the wrong tree.

I'm new to redux/react but would humbly ask this: if composed reducers accessing each other's state is an anti-pattern because it adds coupling among disparate areas of the state shape, I would argue this coupling is already taking place in mapStateToProps() of connect(), since that function provides the root state and components pull from everywhere/anywhere to get their props.

If state subtrees are to be encapsulated, components should have to access state through accessors that hide those subtrees, so that state shape internal to those scopes can be managed without refactoring.

As I learn redux, I am struggling with what I envision to be a long term problem with my app, which is tight coupling of state shape across the app - not by reducers but by mapStateToProps - such as a comment sidebar that has to know the auth user's name (each reduced by a comment reducer vs auth reducer, for instance.) I am experimenting with wrapping all state access in accessor methods that do this kind of encapsulation, even in mapStateToProps, but feel like I might be barking up the wrong tree.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Apr 17, 2016

Contributor

@jwhiting : that's one of the several purposes of using "selector functions" to extract the data you need from state, particularly in mapStateToProps. You can hide the state of your shape so that there's a minimal number of places that actually need to know where state.some.nested.field is. If you haven't seen it yet, go check out the Reselect utility library, and the Computing Derived Data section of the Redux docs.

Contributor

markerikson commented Apr 17, 2016

@jwhiting : that's one of the several purposes of using "selector functions" to extract the data you need from state, particularly in mapStateToProps. You can hide the state of your shape so that there's a minimal number of places that actually need to know where state.some.nested.field is. If you haven't seen it yet, go check out the Reselect utility library, and the Computing Derived Data section of the Redux docs.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 17, 2016

Collaborator

Yeah. You also don’t have to use Reselect if your data volume is small but still use (hand-written) selectors. Check out the shopping-cart example for this approach.

Collaborator

gaearon commented Apr 17, 2016

Yeah. You also don’t have to use Reselect if your data volume is small but still use (hand-written) selectors. Check out the shopping-cart example for this approach.

@jwhiting

This comment has been minimized.

Show comment
Hide comment
@jwhiting

jwhiting Apr 17, 2016

@markerikson @gaearon that makes sense and I will explore that, thank you. Using selectors in mapStateToProps for anything outside that component's primary concern (or perhaps for all state access) would shield components from changes in foreign state shape. I would like to find a way to make state encapsulation more strictly enforced in my project, though, so that code discipline is not the only guard for it and welcome suggestions there.

@markerikson @gaearon that makes sense and I will explore that, thank you. Using selectors in mapStateToProps for anything outside that component's primary concern (or perhaps for all state access) would shield components from changes in foreign state shape. I would like to find a way to make state encapsulation more strictly enforced in my project, though, so that code discipline is not the only guard for it and welcome suggestions there.

@kurtharriger

This comment has been minimized.

Show comment
Hide comment
@kurtharriger

kurtharriger Apr 17, 2016

Keeping all code in a single reducer is not really good separation of
concerns. If your reducer is called by the root you could call it manually
and explicitly pass the root state as I think your suggesting but if your
parent reducer hierarchy contains combineReducers you have to go rip it
out. So why not make it more compossible so that you don't need to rip it
out later or resort to thunks as most people seem to do?
On Sat, Apr 16, 2016 at 8:27 PM Josh Whiting notifications@github.com
wrote:

@markerikson https://github.com/markerikson @gaearon
https://github.com/gaearon that makes sense and I will explore that,
thank you. Using selectors in mapStateToProps for anything outside that
component's primary concern (or perhaps for all state access) would shield
components from changes in foreign state shape. I would like to find a way
to make state encapsulation more strictly enforced in my project, though,
so that code discipline is not the only guard for it and welcome
suggestions there.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
reactjs#601 (comment)

Keeping all code in a single reducer is not really good separation of
concerns. If your reducer is called by the root you could call it manually
and explicitly pass the root state as I think your suggesting but if your
parent reducer hierarchy contains combineReducers you have to go rip it
out. So why not make it more compossible so that you don't need to rip it
out later or resort to thunks as most people seem to do?
On Sat, Apr 16, 2016 at 8:27 PM Josh Whiting notifications@github.com
wrote:

@markerikson https://github.com/markerikson @gaearon
https://github.com/gaearon that makes sense and I will explore that,
thank you. Using selectors in mapStateToProps for anything outside that
component's primary concern (or perhaps for all state access) would shield
components from changes in foreign state shape. I would like to find a way
to make state encapsulation more strictly enforced in my project, though,
so that code discipline is not the only guard for it and welcome
suggestions there.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
reactjs#601 (comment)

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Apr 17, 2016

Contributor

@kurtharriger : we all agree that keeping every last bit of reducer logic in one single function is a bad idea, and that the work should be delegated to sub-functions in some way. However, everyone is going to have different ideas on how to do that, and different needs for their own application. Redux simply provides combineReducers as a built-in utility for a common use case. If you don't want to use that utility as it currently exists, you don't have to - you're entirely welcome to structure your reducers your own way, whether it be writing everything by hand, or writing your own variation of combineReducers that does things in a way that is more suitable for your own needs.

Contributor

markerikson commented Apr 17, 2016

@kurtharriger : we all agree that keeping every last bit of reducer logic in one single function is a bad idea, and that the work should be delegated to sub-functions in some way. However, everyone is going to have different ideas on how to do that, and different needs for their own application. Redux simply provides combineReducers as a built-in utility for a common use case. If you don't want to use that utility as it currently exists, you don't have to - you're entirely welcome to structure your reducers your own way, whether it be writing everything by hand, or writing your own variation of combineReducers that does things in a way that is more suitable for your own needs.

@onethread

This comment has been minimized.

Show comment
Hide comment
@onethread

onethread Apr 19, 2016

Since we're speaking on patterns and separation of concerns, I'm actually running into something related, but with actions instead.

Namely, the ajax shouldFetch pattern I see around in various places seems to be a tight coupling to the state shape. For example, here. In that example, what if I reuse the reducer but do not have the entities object at the root of the state? That action would then fail. What's the recommendation here? Should I add case-specific actions? Or an action that accepts a selector?

Since we're speaking on patterns and separation of concerns, I'm actually running into something related, but with actions instead.

Namely, the ajax shouldFetch pattern I see around in various places seems to be a tight coupling to the state shape. For example, here. In that example, what if I reuse the reducer but do not have the entities object at the root of the state? That action would then fail. What's the recommendation here? Should I add case-specific actions? Or an action that accepts a selector?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2016

Collaborator

@Shadowii Having an action creator import a selector seems fine to me. I’d colocate selectors with reducers so knowledge about state shape is localized in those files.

Collaborator

gaearon commented Apr 19, 2016

@Shadowii Having an action creator import a selector seems fine to me. I’d colocate selectors with reducers so knowledge about state shape is localized in those files.

@fabiosussetto

This comment has been minimized.

Show comment
Hide comment
@fabiosussetto

fabiosussetto May 18, 2016

What's the problem with having different reducers handle the same action type?
For example, if I have a account and a auth reducers, they both handle the LOGIN_SUCCESS action type (with the auth token and the account data in the payload), each one updating only the slice of state they manage. I've found myself using this approach quite a few times when an action affects different parts of the state.

What's the problem with having different reducers handle the same action type?
For example, if I have a account and a auth reducers, they both handle the LOGIN_SUCCESS action type (with the auth token and the account data in the payload), each one updating only the slice of state they manage. I've found myself using this approach quite a few times when an action affects different parts of the state.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 19, 2016

Collaborator

What's the problem with having different reducers handle the same action type?

There is no problem, it’s a very common and useful pattern. In fact it’s the reason actions exist: if actions mapped to reducers 1:1, there’d be little point in having actions at all.

Collaborator

gaearon commented May 19, 2016

What's the problem with having different reducers handle the same action type?

There is no problem, it’s a very common and useful pattern. In fact it’s the reason actions exist: if actions mapped to reducers 1:1, there’d be little point in having actions at all.

@markerikson markerikson referenced this issue Sep 18, 2016

Closed

Add a "Structuring Reducers" recipe #1784

12 of 12 tasks complete
@mars76

This comment has been minimized.

Show comment
Hide comment
@mars76

mars76 Nov 4, 2016

@rhys-vdw i tried using the middleware you posted.

customMiddleare.js

const customMiddleware =  store => next => action => {
  next({ ...action, getState: store.getState });
};

and in my store creation i have

import customMiddleware from './customMiddleware';

var store = createStore(
        rootReducer,
        initialState,
        applyMiddleware(
            reduxPromise,
            thunkMiddleware,
            loggerMiddleware,
            customMiddleware
        )
    );
return store;

I am getting the following Error:

applyMiddleware.js?ee15:49 Uncaught TypeError: middleware is not a function(…)
(anonymous function) @ applyMiddleware.js?ee15:49
(anonymous function) @ applyMiddleware.js?ee15:48
createStore @ createStore.js?fe4c:65
configureStore @ configureStore.js?ffde:45
(anonymous function) @ index.jsx?fdd7:25
(anonymous function) @ bundle.js:1639
webpack_require @ bundle.js:556
fn @ bundle.js:87(anonymous function) @ bundle.js:588
webpack_require @ bundle.js:556
(anonymous function) @ bundle.js:579
(anonymous function) @ bundle.js:582

mars76 commented Nov 4, 2016

@rhys-vdw i tried using the middleware you posted.

customMiddleare.js

const customMiddleware =  store => next => action => {
  next({ ...action, getState: store.getState });
};

and in my store creation i have

import customMiddleware from './customMiddleware';

var store = createStore(
        rootReducer,
        initialState,
        applyMiddleware(
            reduxPromise,
            thunkMiddleware,
            loggerMiddleware,
            customMiddleware
        )
    );
return store;

I am getting the following Error:

applyMiddleware.js?ee15:49 Uncaught TypeError: middleware is not a function(…)
(anonymous function) @ applyMiddleware.js?ee15:49
(anonymous function) @ applyMiddleware.js?ee15:48
createStore @ createStore.js?fe4c:65
configureStore @ configureStore.js?ffde:45
(anonymous function) @ index.jsx?fdd7:25
(anonymous function) @ bundle.js:1639
webpack_require @ bundle.js:556
fn @ bundle.js:87(anonymous function) @ bundle.js:588
webpack_require @ bundle.js:556
(anonymous function) @ bundle.js:579
(anonymous function) @ bundle.js:582

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Nov 4, 2016

Contributor

@mars76 : Most likely you're not importing something correctly. If that really is the entire contents of customMiddleware.js, then you're not exporting anything. In general, you're passing four middlewares to applyMiddleware, and presumably one of them is not a valid reference. Step back, remove all of them, re-add one at a time, see which one is not valid, and figure out why. Check import and export statements, double-check how you're supposed to initialize things, etc.

Contributor

markerikson commented Nov 4, 2016

@mars76 : Most likely you're not importing something correctly. If that really is the entire contents of customMiddleware.js, then you're not exporting anything. In general, you're passing four middlewares to applyMiddleware, and presumably one of them is not a valid reference. Step back, remove all of them, re-add one at a time, see which one is not valid, and figure out why. Check import and export statements, double-check how you're supposed to initialize things, etc.

@mars76

This comment has been minimized.

Show comment
Hide comment
@mars76

mars76 Nov 4, 2016

Hi,

Thanks @markerikson

I converted the syntax as below and it's fine now:

customMiddleware.js

export function customMiddleware(store) { return function (next) { return function (action) { next(Object.assign({}, action, { getState: store.getState })); }; }; } console.log(customMiddleware);

How ever i am trying to access the store in the Action Creator itself (Not in the Reducer). I need to make an Async call and need to pass different parts of the store's state managed by various reducers.

This is what i am doing. Not sure whether this is the right approach.

I dispatch an action with the data and inside the reducer i have now access to the entire State and i am extracting the necessary parts which i need and creating an Object and dispatching another action with this data which will be available in my Action Creator where i am making an Asyn call using that data.

My biggest concern is Reducers are now calling action creator methods.

Appreciate any comments.

Thanks

mars76 commented Nov 4, 2016

Hi,

Thanks @markerikson

I converted the syntax as below and it's fine now:

customMiddleware.js

export function customMiddleware(store) { return function (next) { return function (action) { next(Object.assign({}, action, { getState: store.getState })); }; }; } console.log(customMiddleware);

How ever i am trying to access the store in the Action Creator itself (Not in the Reducer). I need to make an Async call and need to pass different parts of the store's state managed by various reducers.

This is what i am doing. Not sure whether this is the right approach.

I dispatch an action with the data and inside the reducer i have now access to the entire State and i am extracting the necessary parts which i need and creating an Object and dispatching another action with this data which will be available in my Action Creator where i am making an Asyn call using that data.

My biggest concern is Reducers are now calling action creator methods.

Appreciate any comments.

Thanks

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Nov 4, 2016

Contributor

@mars76 : again, you should _not ever_ try to dispatch from within a Reducer.

For accessing the store inside your action creators, you can use the redux-thunk middleware. You might also want to read the FAQ question on sharing data between reducers, as well as a gist I wrote with some examples of common thunk usages.

Contributor

markerikson commented Nov 4, 2016

@mars76 : again, you should _not ever_ try to dispatch from within a Reducer.

For accessing the store inside your action creators, you can use the redux-thunk middleware. You might also want to read the FAQ question on sharing data between reducers, as well as a gist I wrote with some examples of common thunk usages.

@mars76

This comment has been minimized.

Show comment
Hide comment
@mars76

mars76 Nov 4, 2016

Thanks a lot @markerikson.

My bad.I didn't realize about the getState() in redux-thunk. That make my job much easier and leave my reducers pure.

mars76 commented Nov 4, 2016

Thanks a lot @markerikson.

My bad.I didn't realize about the getState() in redux-thunk. That make my job much easier and leave my reducers pure.

@tacomanator

This comment has been minimized.

Show comment
Hide comment
@tacomanator

tacomanator Jan 6, 2017

@gaearon when you mentioned having an action creator import a selector, was it in your mind that the entire state would be passed to the action creator for handoff to the selector, or am I missing something?

@gaearon when you mentioned having an action creator import a selector, was it in your mind that the entire state would be passed to the action creator for handoff to the selector, or am I missing something?

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jan 6, 2017

Contributor

@tacomanator : FYI, Dan doesn't usually respond to random pings in Redux issues these days. Too many other priorities.

Most of the time, if you're using a selector in an action creator, you're doing it inside of a thunk. Thunks have access to getState, so the entire state tree is accessible:

import {someSelector} from "./selectors";

function someThunk(someParameter) {
    return (dispatch, getState) => {
        const specificData = someSelector(getState(), someParameter);

        dispatch({type : "SOME_ACTION", payload : specificData});    
    }
}
Contributor

markerikson commented Jan 6, 2017

@tacomanator : FYI, Dan doesn't usually respond to random pings in Redux issues these days. Too many other priorities.

Most of the time, if you're using a selector in an action creator, you're doing it inside of a thunk. Thunks have access to getState, so the entire state tree is accessible:

import {someSelector} from "./selectors";

function someThunk(someParameter) {
    return (dispatch, getState) => {
        const specificData = someSelector(getState(), someParameter);

        dispatch({type : "SOME_ACTION", payload : specificData});    
    }
}
@tacomanator

This comment has been minimized.

Show comment
Hide comment
@tacomanator

tacomanator Jan 6, 2017

@markerikson thanks, that makes sense. FYI the reason I'm asking is mostly about using the data derived by selectors for validation business logic as opposed to dispatching it to a reducer, but that also gives me food for thought.

@markerikson thanks, that makes sense. FYI the reason I'm asking is mostly about using the data derived by selectors for validation business logic as opposed to dispatching it to a reducer, but that also gives me food for thought.

@brianpkelley

This comment has been minimized.

Show comment
Hide comment
@brianpkelley

brianpkelley Feb 24, 2017

A quick hack I used to combine reducers into a single one was

const reducers = [ reducerA, reducerB, ..., reducerZ ];

let reducer = ( state = initialState, action ) => {
	return reducers.reduce( ( state, reducerFn ) => (
		reducerFn( state, action )
	), state );
};

where reducerA through reducerZ are the individual reducer functions.

brianpkelley commented Feb 24, 2017

A quick hack I used to combine reducers into a single one was

const reducers = [ reducerA, reducerB, ..., reducerZ ];

let reducer = ( state = initialState, action ) => {
	return reducers.reduce( ( state, reducerFn ) => (
		reducerFn( state, action )
	), state );
};

where reducerA through reducerZ are the individual reducer functions.

@markerikson

This comment has been minimized.

Show comment
Hide comment
Contributor

markerikson commented Feb 24, 2017

@brianpkelley

This comment has been minimized.

Show comment
Hide comment
@brianpkelley

brianpkelley Feb 24, 2017

@markerikson Ah nice, first time using react/redux/etc and found this thread while looking into this problem. Skipped to the end around 1/2 way through lol

@markerikson Ah nice, first time using react/redux/etc and found this thread while looking into this problem. Skipped to the end around 1/2 way through lol

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Feb 24, 2017

Contributor

Heh, sure.

FYI, you might want to read through the FAQ: "Do I have to use combineReducers?" and Structuring Reducers sections in the docs. Also, my "Practical Redux" tutorial series has a couple posts that demonstrate some advanced reducer techniques (see Part 7 and Part 8).

Contributor

markerikson commented Feb 24, 2017

Heh, sure.

FYI, you might want to read through the FAQ: "Do I have to use combineReducers?" and Structuring Reducers sections in the docs. Also, my "Practical Redux" tutorial series has a couple posts that demonstrate some advanced reducer techniques (see Part 7 and Part 8).

outoftime added a commit to outoftime/popcode that referenced this issue Apr 3, 2017

Projects reducer consumes `USER_LOGGED_OUT`
When the user logs out, we want to clear all projects out of the list
except for the current project. This requires the projects reducer to
know what the current project is.

There are several ways to solve this problem without giving the projects
reducer access to the rest of the store:

1. Have the current project key passed in the action payload by the
   point of dispatch (e.g. from the Workspace component)
2. Use a thunk action creator that can introspect the current state and
   then add the current project key to a dispatched action payload
3. Duplicate the information in the `currentProject` subtree, also
   marking the object in `projects` as `current`
4. Don’t bother trimming the projects store–just enforce a rule that only
   the current project is visible using a selector

However, each of these approaches has significant disadvantages:

1. The fact that a reducer needs to know about the current project when
   consuming `USER_LOGGED_OUT` is an implementation detail of the store;
   the component that initially dispatches the action should not need to
   know this
2. Thunk action creators are considered harmful and are being removed
   from our code
3. It’s a very good idea to keep the Redux store fully normalized.
4. This approach would lead to an incoherent store state, and we’d have
   roughly the same problem when the user logs in.

Contra the author of [this highly upvoted GitHub issue
comment](reactjs/redux#601 (comment)),
I don’t think it’s an antipattern for reducers to have access to the
entire store. In fact, this is the great strength of Redux—we’re able to
model all state transitions based on a universally-agreed-upon answer to
the question “what is the current state of the world?” The
`combineReducers` approach to isolating the reducer logic for different
parts of the subtree is a very useful tool for organizing code; it’s not
a mandate to only organize code that way.

Further, options 1 and 2 above feel a bit ridiculous because,
fundamentally, **reducers do have access to the entire state**. Why
would we jump through hoops just to give the reducer information it
already has access to?

So: establish a pattern that reducer modules may export a named
`reduceRoot` function, which takes the entire state and performs
reductions on it. The top-level root reducer will import this function
and apply it to the state *after running the isolated reducers* using
the `reduce-reducers` module.

outoftime added a commit to outoftime/popcode that referenced this issue Apr 3, 2017

Projects reducer consumes `USER_LOGGED_OUT`
When the user logs out, we want to clear all projects out of the list
except for the current project. This requires the projects reducer to
know what the current project is.

There are several ways to solve this problem without giving the projects
reducer access to the rest of the store:

1. Have the current project key passed in the action payload by the
   point of dispatch (e.g. from the Workspace component)
2. Use a thunk action creator that can introspect the current state and
   then add the current project key to a dispatched action payload
3. Duplicate the information in the `currentProject` subtree, also
   marking the object in `projects` as `current`
4. Don’t bother trimming the projects store–just enforce a rule that only
   the current project is visible using a selector

However, each of these approaches has significant disadvantages:

1. The fact that a reducer needs to know about the current project when
   consuming `USER_LOGGED_OUT` is an implementation detail of the store;
   the component that initially dispatches the action should not need to
   know this
2. Thunk action creators are considered harmful and are being removed
   from our code
3. It’s a very good idea to keep the Redux store fully normalized.
4. This approach would lead to an incoherent store state, and we’d have
   roughly the same problem when the user logs in.

Contra the author of [this highly upvoted GitHub issue
comment](reactjs/redux#601 (comment)),
I don’t think it’s an antipattern for reducers to have access to the
entire store. In fact, this is the great strength of Redux—we’re able to
model all state transitions based on a universally-agreed-upon answer to
the question “what is the current state of the world?” The
`combineReducers` approach to isolating the reducer logic for different
parts of the subtree is a very useful tool for organizing code; it’s not
a mandate to only organize code that way.

Further, options 1 and 2 above feel a bit ridiculous because,
fundamentally, **reducers do have access to the entire state**. Why
would we jump through hoops just to give the reducer information it
already has access to?

So: establish a pattern that reducer modules may export a named
`reduceRoot` function, which takes the entire state and performs
reductions on it. The top-level root reducer will import this function
and apply it to the state *after running the isolated reducers* using
the `reduce-reducers` module.

@outoftime outoftime referenced this issue in popcodeorg/popcode Apr 3, 2017

Merged

Logout handled by reducers #753

sfentress added a commit to concord-consortium/geniblocks that referenced this issue Jun 21, 2017

Move fertilize and breed actions to drake reducer
Part of work to clean up the root reducer to make further refactoring
easier.

Both these actions only affect the `drakes` property, so can be handled
entirely by the drakes reducer.

The one issue is that `fertilize` needs additional information from
elsewhere in the state tree, namely the gametes. While we could maybe
completely restructure the state, or add an all-purpose way for all
reducers to get the entire state (reduxjs/redux#601 (comment))
it seemed more natural simply to let the drakes reducer know about the
gametes. This pattern can be applied elsewhere when a reducer is only
updating one property, but needs knowledge about other state props.
@zdila

This comment has been minimized.

Show comment
Hide comment
@zdila

zdila Aug 23, 2017

My two cents when using redux-logic is to augment the action from the root state in transform function.

Example:

const formInitLogic = createLogic({
  type: 'FORM_INIT',
  transform({ getState, action }, next) {
    const state = getState();
    next({
      ...action,
      payload: {
        ...action.payload,
        property1: state.branch1.someProperty > 5,
        property2: state.branch1.anotherProperty + state.branch2.yetAnotherProperty,
      },
    });
  },
});

zdila commented Aug 23, 2017

My two cents when using redux-logic is to augment the action from the root state in transform function.

Example:

const formInitLogic = createLogic({
  type: 'FORM_INIT',
  transform({ getState, action }, next) {
    const state = getState();
    next({
      ...action,
      payload: {
        ...action.payload,
        property1: state.branch1.someProperty > 5,
        property2: state.branch1.anotherProperty + state.branch2.yetAnotherProperty,
      },
    });
  },
});
@digital-flowers

This comment has been minimized.

Show comment
Hide comment
@digital-flowers

digital-flowers Sep 23, 2017

i think it is more about code structure :
"don't access substate actions directly"
after using redux in many projects i found that the best code structure is to deal with every sub state as a completely separated component every one of these components has it is own folder and logic, to implement this idea i use the following files structure:

  • redux root
    • reducers.js export combine reducers from every state
    • middleware.js export applyMiddleware for all middleware
    • configureStore.js createStore using (reducers.js, middleware.js)
    • actions.js export actions that dispatch actions from states folders <== THIS
    • state1
      • initial.js this file contains the initial state (i use immutable to create a record)
      • action-types.js this file contains the action types (i use dacho as key mirror)
      • actions.js this file contains the state actions
      • reducer.js this file contains the state reducer
    • state2
      • initial.js
      • action-types.js
        ....

Why ?
1- there is 2 types of actions in every application:

  • state actions (redux/state1/actions.js) these actions responsibility to only change state, you shouldn't fire these actions directly always use app actions
  • app actions (redux/actions.js) these actions responsibility to do some app logic and can fire state actions

2- additionally using this structure you can create a new state just by copy paste a folder and rename it ;)

digital-flowers commented Sep 23, 2017

i think it is more about code structure :
"don't access substate actions directly"
after using redux in many projects i found that the best code structure is to deal with every sub state as a completely separated component every one of these components has it is own folder and logic, to implement this idea i use the following files structure:

  • redux root
    • reducers.js export combine reducers from every state
    • middleware.js export applyMiddleware for all middleware
    • configureStore.js createStore using (reducers.js, middleware.js)
    • actions.js export actions that dispatch actions from states folders <== THIS
    • state1
      • initial.js this file contains the initial state (i use immutable to create a record)
      • action-types.js this file contains the action types (i use dacho as key mirror)
      • actions.js this file contains the state actions
      • reducer.js this file contains the state reducer
    • state2
      • initial.js
      • action-types.js
        ....

Why ?
1- there is 2 types of actions in every application:

  • state actions (redux/state1/actions.js) these actions responsibility to only change state, you shouldn't fire these actions directly always use app actions
  • app actions (redux/actions.js) these actions responsibility to do some app logic and can fire state actions

2- additionally using this structure you can create a new state just by copy paste a folder and rename it ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment