ngrx / store

RxJS powered state management for Angular applications, inspired by Redux
MIT License
3.9k stars 311 forks source link

Another CombineReducers method #397

Closed EddyP23 closed 7 years ago

EddyP23 commented 7 years ago

This is a proposal for enhancement.

There is a combineReducers function in utils that combines reducers that act upon elements in the object itself.

Say I have the following object:

let StoreState = {
param1: string,
param2: string
}

and then I have two reducers that act on param1 and param2 respectively.

What I am looking at is having a different way of combining reducers that could combine a reducer that acts upon the whole StoreState object itself together with reducers that act on param1 and param2 separately.

We have some actions that want to manage state of the whole StoreState objects and some that need manage sub-states. We could solve this using effects but it feels that it would over-complicate the design.

kylecordes commented 7 years ago

The output of combineReducers is a function. You can wrap another function around that, which then serves as a reducer for the entire combined state. Is there a need for a utility to help do so? It already seems pretty trivial.

(One of the common misuse patterns we see is Effects that only consume actions and emit other actions to affect the state - without any interaction with the outside world. Such logic is better and more simply implemented in a reducer of course.)

EddyP23 commented 7 years ago

@kylecordes thanks for the swift reply and an overall comment to the problem. :)

I would think that there is a place for it in the utils as it seems to be a pretty common task. Why don't you like adding that to the utils?

kylecordes commented 7 years ago

@EddyP23 I don't see how there is anything you could add to utils that would make it any easier though. You just write a function which delegates to the output of combineReducers before or after doing its own overall reduction work.

However, this should probably be visited again once the exciting new version ships with support in the box for lazy loaded additional reducers. Once that is in place, it probably would be helpful to have some explicit, named way to hand the machinery an overall reducer.

EddyP23 commented 7 years ago

@kylecordes Yeah, I think I get what you mean know.

The remaining issue we have is that if there are some properties that child reducers don't act on, they will get removed in a new state construction, in line from utils:

return hasChanged ? nextState : state;

What we have now in our utils 'fork' is:

return hasChanged ? Object.assign({}, state, nextChildState) : state;

This way we leave all properties of state that are not touched by child reducers the way they are and our parent reducer looks like this:

export function parentReducer(state = initialState, action: any): ParentState {
    if (action.type === SomeAction) {
        // do whatever
    }
    return combineReducers(state, action);
}

Maybe this is something you think of including in the utils.ts?

robwormald commented 7 years ago

Please check this against NgRx v4, and if it’s still an issue, please reopen on https://github.com/ngrx/platform. Thanks!