Immutability issue

[maxime1992]

Hi Victor, after watching your talk about tackling state in Angular apps I wanted to dig into the code of this repo and while I was reading your reducer I noticed an immutability issue. Checkout model.ts#L49.

case 'RATE': {
  const talks = {...state.talks};
  talks[action.payload.talkId].rating = action.payload.rating; // here a mutation is happening
  return {...state, talks};
}

And it should be

case 'RATE': {
  const talks = {...state.talks};
  talks[action.payload.talkId] = {
    ...state.talks[action.payload.talkId],
    rating: action.payload.rating
  };
  return {...state, talks};
}

Or, I have a preference for this syntax

case 'RATE': {
  return {
    ...state,
    talks: {
      ...state.talks,
      [action.payload.talkId]: {
        ...state.talks[action.payload.talkId],
        rating: action.payload.rating
      }
    }
  }
}

I made a quick repro of the bug that you can run in chrome console

var state = {
  talks: {
    talkId0: { id: 'talkId0', title: 'talk 0', rating: 3 },
    talkId1: { id: 'talkId1', title: 'talk 1', rating: 3 },
  }
};

// talk_updated
var talks = Object.assign({}, state.talks);
talks['talkId0'].rating = 4;

console.log(state.talks['talkId0']); // here notice that the initial state object has been mutated!

BTW your talk was great and I can't wait to give a try to ngrx v4 :).

Cheers

[apasternack]

Your first suggested refactoring looks mutable to me:

case 'RATE': {
  const talks = {...state.talks};
  talks[action.payload.talkId] = {
    ...state.talks[action.payload.talkId],
    rating: action.payload.rating
  };
  return {...state, talks};
}

You just mutate the talks object one level up in the nesting. Am I missing something?

The last refactoring you have looks immutable to me. So the change being more than mere syntax preference:

case 'RATE': {
  return {
    ...state,
    talks: {
      ...state.talks,
      [action.payload.talkId]: {
        ...state.talks[action.payload.talkId],
        rating: action.payload.rating
      }
    }
  }
}

[maxime1992]

Is this a question for me @apasternack ? Because it's all what's this issue is about.

The first "suggested refactoring" is the code from the talk and the repo.

The second one is my proposal (which is exactly the same as your last part in comment) so I do not understand.

[apasternack]

Hello @maxime1992, yeah it was a question. I was confused. I thought that first "suggested refactoring" was from you. Now it makes sense! Thanks, wanted to make sure I understood the bit about avoiding mutating state.