Anti-pattern: Mutating state and action payload

[gaearon]

Thanks for using Redux!

This example has a significant problem: its reducers aren't pure. For example, state.products is mutated on this line:

                state.products.push(product);

This goes against how Redux works: you should always return new state instead of mutating the state passed by Redux.

Another anti-pattern is above it:

            var product = action.product;
            var id = product.id;

            if( state.products.indexOf(product) != -1) {
                product.quantity++;

It has two problems:

1. The action payload action.product is mutated (product.quantity++). The action payload should never be mutated as this destroys the history.
2. The reducer relies on the fact that action.product is one of the items inside state.products. This is not currently documented (note to me!), but it's also an anti-pattern. You should never rely on something inside action being the same object as something in your state. This is because action is meant to be a description. It can be serialized, and deserialized again, and your reducers should work exactly the same.

When you feel like you want to pass objects like product in an action, it's a sign you really want to store an ID map, and pass productId inside the action. See Redux TodoMVC reducer as an example of that:

const initialState = [{
  text: 'Use Redux',
  marked: false,
  id: 0
}];

export default function todos(state = initialState, action) {
  switch (action.type) {
  case ADD_TODO:
    return [{
      id: (state.length === 0) ? 0 : state[0].id + 1,
      marked: false,
      text: action.text
    }, ...state];

  case DELETE_TODO:
    return state.filter(todo =>
      todo.id !== action.id
    );

  case EDIT_TODO:
    return state.map(todo =>
      todo.id === action.id ?
        Object.assign({}, todo, { text: action.text }) :
        todo
    );

  case MARK_TODO:
    return state.map(todo =>
      todo.id === action.id ?
        Object.assign({}, todo, { marked: !todo.marked }) :
        todo
    );

  case MARK_ALL:
    const areAllMarked = state.every(todo => todo.marked);
    return state.map(todo => Object.assign({}, todo, {
      marked: !areAllMarked
    }));

  case CLEAR_MARKED:
    return state.filter(todo => todo.marked === false);

  default:
    return state;
  }
}

Note that it never writes to the state, and it also never relies on a particular object being equal to something inside the state.

These anti-patterns are likely the reason DevTools didn't work for you (6998c46). They assume the reducers are pure.

We'll make sure to document these anti-patterns and how to avoid them.
Thanks again!

[coodoo]

Thanks @gaearon for the detailed instructions, I have revised the code according and pushed the bits.

Major changes include:

1. Normalized the data structure so now carts just stored id of purchased products
2. Revised ADD_TO_CART logics in products so that it always look up product by id and never modify the state passed to it

In the future I might as well just use immutablejs to maintain the immutability (which comes with better fool-proof capability).

After the rewrite, redux-devtools is confirmed to working as expected!

@gaearon would you mind reviewing the revision again? thanks.

[gaearon]

Everything looks good now! Thanks for addressing all of these so quickly.