Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make combineReducers more flexible about additional reducers' arguments #879

Closed
wants to merge 1 commit into from
Closed

Make combineReducers more flexible about additional reducers' arguments #879

wants to merge 1 commit into from

Conversation

mdziekon
Copy link

Hi, a couple of days ago, when searching for an answer to my problem with inaccessible state data inside of nested reducers (created with combineReducers and other helpers), I've found a similar question with a solution/proposition to pass down additional arguments to the reducers, with required data to calculate state change.

Unfortunately, combineReducers generates combined reducer, which do not pass any arguments besides the previous state and any passed action to the nested reducers. My PR fixes this problem by simply passing "rest" of the arguments to other reducers (using spread operator).

I hope this is a valuable change to the code-base (well, it definitely helped me a lot to decouple my code into smaller chunks), if there is anything else to do about it (like changing the docs or writing some tests), please just let me know.

@gaearon
Copy link
Contributor

gaearon commented Oct 11, 2015

Can you show a specific usage example?

@mdziekon
Copy link
Author

Ok, so this example describes an app, which have a concept of rooms for users/players - each user can create, join and leave a room, so we have to know where each player currently is. Each room has a "owner", who should have higher privileges than the other users in the room (eg. users kicking privilege).
The thing is, when the owner leaves the room, we would like to select another owner, who said he wants to be an owner (it's an "opt-in" setting).

For simplicity, let's assume we've already checked (somewhere, maybe in a middleware) if that can be accomplished (there is at least one user in the room who has accepted to be the next owner).

  • Let's say we have the following state structure in our app:
// Structure

{
    players: {
        1: {
            canBeOwner: true
        },
        2: {
            canBeOwner: false
        },
        123: {
            canBeOwner: true
        },
        256: {
            canBeOwner: false
        }
    },
    rooms: {
        0: {
            data: {
                name: "my room",
                created: "2015-10-10",
                ownerID: 123
            },
            players: [
                1, 2, 123, 256
            ]
        }
    }
}
  • We have following action:
    ROOM_LEAVE
    { type: 'ROOM_LEAVE', roomID: Int }
  • And there are the reducers (with applied combineReducers fix):
// Reducers

// ... roomsMapReducer
// higher-order reducer passes "globalPlayers" object,
// which is an equivalent of "store.getState().players"
function roomsMapReducer(state, action, { globalPlayers }) {
    const roomID = action.roomID;

    switch (action.type) {
        case 'ROOM_LEAVE':
            return {
                ...state,
                [roomID]: roomReducers(state[roomID], action, {
                    roomPlayers: state[roomID].players,
                    globalPlayers
                });
            };
        default:
            return state;
    }
}

// ... roomReducer
// imported "data" (alias for "roomDataReducer") and "players" reducers

combineReducers({
    data,
    players // Irrelevant in this example
})

// ... roomDataReducer (data)
function roomDataReducer(state, action, { roomPlayers, globalPlayers }) {
    switch (action.type) {
        case 'ROOM_LEAVE':
            // Pick possible next owners
            const nextOwnersIDs = _.without(_.keys(roomPlayers), state.ownerID);

            // Pick first player who can be an owner
            const nextOwnerID = _.find(nextOwnersIDs, (ownerID) => {
                return globalPlayers[ownerID].canBeOwner;
            });

            return {
                ...state,
                ownerID: nextOwnerID
            };
        default:
            return state;
    }
}

As you can see, roomDataReducer uses state pieces that are stored somewhere in the tree, but it has no access to it because of it's nesting. Without my PR, combineReducers would not pass down the additional arguments down to the other reducers.

Hope this is actually a valid use case (or, my structure is a valid "redux" structure) which describes "the need" of my fix. If I'm wrong, please let me know (and I guess I'll have to reconsider my structure).

@gaearon
Copy link
Contributor

gaearon commented Oct 15, 2015

I see where you're coming from, but I'm inclined to close this.

combineReducers() was never intended as one-size-fits-all utility. When you need to go “a little bit more custom” you should make your own, whether a higher order one (like combineReducers() itself) or calling your reducers directly:

function root(state = {}, action) {
  return {
    stuff: stuff(state.stuff, action, state),
    otherStuff: stuff(state.otherStuff, action, state)
  };
}

This makes the pattern you're using explicit, and it stands out to somebody who might be puzzled by the last argument they haven't seen before.

@gaearon gaearon closed this Oct 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants