A concern on combineReducer, when an action is related to multiple reducers #601
Comments
|
Can you elaborate why you think See also: https://github.com/rackt/redux/tree/master/examples/real-world/reducers for inspiration. |
|
The first exmple you pointed about is similar to my case, 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:
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. |
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,
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 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. |
|
I'd like to try that. |
|
I just wrote a demo page to try redux. I think |
|
@jiyinyiyong I'm really not sure what complexity you are talking about, but feel free to not use |
|
I wrote my own 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 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 ( 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 |
|
@rhys-vdw thanks for that! A really helpful middleware :) |
|
@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. |
|
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 |
|
@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 However I also think it would be ace if |
|
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 |
|
Accessing the state of other reducers in a reducer is most often an anti-pattern.
|
|
Thanks! I agree with that. I just found it became more complex after trying to pass the root state into reducers :-) |
|
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. |
|
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? |
|
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. |
|
@kurtharriger : note that while the common recommended pattern is to structure Redux state by domain, and use The Redux FAQ answer on splitting business logic also has some good discussion on this topic. |
|
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. |
|
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. |
|
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. |
|
@jwhiting : that's one of the several purposes of using "selector functions" to extract the data you need from state, particularly in |
|
Yeah. You also don’t have to use Reselect if your data volume is small but still use (hand-written) selectors. Check out the |
|
@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. |
|
Keeping all code in a single reducer is not really good separation of
|
|
@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 |
|
@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. |
|
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. |
|
@rhys-vdw i tried using the middleware you posted. customMiddleare.js and in my store creation i have I am getting the following Error: applyMiddleware.js?ee15:49 Uncaught TypeError: middleware is not a function(…) |
|
@mars76 : Most likely you're not importing something correctly. If that really is the entire contents of |
|
Hi, Thanks @markerikson I converted the syntax as below and it's fine now: customMiddleware.js
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 : 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. |
|
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. |
|
@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? |
|
@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 import {someSelector} from "./selectors";
function someThunk(someParameter) {
return (dispatch, getState) => {
const specificData = someSelector(getState(), someParameter);
dispatch({type : "SOME_ACTION", payload : specificData});
}
} |
|
@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. |
|
A quick hack I used to combine reducers into a single one was where |
|
@brianpkelley : yep, that 's basically https://github.com/acdlite/reduce-reducers . |
|
@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 |
|
Heh, sure. FYI, you might want to read through the FAQ: "Do I have to use |
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](reduxjs/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.
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](reduxjs/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.
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.
|
My two cents when using redux-logic is to augment the action from the root state in 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,
},
});
},
}); |
|
i think it is more about code structure :
Why ?
2- additionally using this structure you can create a new state just by copy paste a folder and rename it ;) |
|
What about when 2 reducers need to change the same property? Let's say that I have a reducer that I'd like to split up to different files and that there's a property called 'error', which I change when http requests fail for one reason or another. Now at some point the reducer has become to big, so I decided to split it up to several reducers, but all of them need to change that 'error' property. Then what? |
|
@Wassap124 It's not possible for two reducers to manage the same state. Do you mean two actions need to change the same property? Without further details you might prefer to use a thunk to dispatch a "set error" action. |
|
@rhys-vdw I'm just a bit stuck trying to split up a fairly long reducer. |
|
@Wassap124 , @rhys-vdw : to clarify, it's not possible for two reducers to manage the same state if you're only using |
|
If you do some quick source spelunking... The see https://github.com/reduxjs/redux/blob/master/src/combineReducers.js#L145 after creating a closure to reference the actual reducers object. Therefore, you can add something simple like so to globally handle actions in the case of needing to hydrate and reload state from storage. This may or may not be an anti pattern but pragmatism has always been more important. |
|
Instead of calling a reducer from another reducer (which is an antipattern, because globalReducerHandler has nothing to do with the reducer it calls), use |
I'm working on a chat app built with React.js and Flux. As I read the docs of Redux the
combineReducerfunction appears strange to me. For example:There are three stores called
messageStore,unreadStore,threadStore. And there's and action calledNEW_MESSAGE. All three stores will update on new messages. In Redux, the stores are reducers combined withcombineReducerlike: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:
The text was updated successfully, but these errors were encountered: