React components not updating

[cideM]

I've tried any and everything I came across so now I give up and turn to someone with more expertise. Before I start, here's the repo: https://github.com/cideM/tictactoe (just clone, npm install, npm start, available at localhost:3000)

What's supposed to happen: you click a cell in the grid and then the store gets a new grid object with a new entry (e.g., 1: 'X'). This grid object is saved to the store as "markedFields", which is passed down to the Game component and then down to TableRow and TableCol. TableCol is just a dumb component for , which would normally show either X, O or nothing, all depending on the state of the store.

But without forceUpdate() in my onClickHandler function, the Game component just doesn't update. Everything else appears to be working. I've checked dozens of different ways of storing the grid (flat vs. nested) and I am pretty sure that I am not mutating the state in my reducer ... but maybe my lack of Javascript skills coupled with me having banged my head against the wall for too long now, makes me overlook some glaring issue.

I would really appreciate it if someone could just tell me why nothing updates. Ignore any other issues as this repo is a huge work in progress and it's my first time with redux (and my 2nd or 3rd time with react...).

Thanks!

[gaearon]

This is a mutation:

      var newState = Object.assign({}, state);
      newState.markedFields[action.newField] = action.side;

You are shallowly copying the state object but you still mutate the markedFields because it is copied by reference so you are still modifying the existing markFields object.

You can find this exact scenario in COMPLETE_TODO example in http://redux.js.org/docs/Troubleshooting.html. The fix is to copy anything you change, not just the top-level state. You don't need to copy the parts that weren't changed though.

In general, rather than write exquisite Object.assign calls, we recommend using object spread operator, as described in http://redux.js.org/docs/recipes/UsingObjectSpreadOperator.html.

[markerikson]

Or, one of the dozen immutable update abstraction utilities I have listed in my Redux addons catalog: Immutable Data#Immutable Update Utilities

[markerikson]

I will also point to the related FAQ question, as well, which also describes this specific scenario: http://redux.js.org/docs/FAQ.html#react-not-rerendering

[cideM]

Alright I feel stupid and sorry for wasting everyone's time, because it's clear that I mutated the state by referencing the original state. But there's actually a deeper issue. The GitHub branch you looked at was just one of my attempted solutions (the last one), whereas before that, I didn't mutate the state (in the reducer) and yet I still had that problem.

So maybe this helps someone else. My reducer used to look like his:

case 'MARK_FIELD':
      return Object.assign({}, state, {
        markedFields: action.newGrid
      });

And the function that dispatched the action looked like this:

onClickHandler(e) {
    e.preventDefault();
    let targetID = e.target.id;
    let newGrid = this.props.markedFields;
    newGrid[targetID] = this.props.playerChoice;
    this.props.markField(newGrid);
    console.log(this.props);
  }

I was looking at the reducer the entire time, even though it wasn't the source of the problem. But now that you confirmed (kinda retroactively) that my original reducer was in fact correct, I looked at the rest of my code. I then realized that

let newGrid = this.props.markedFields;

was again referencing the original state. By making the following change, I was finally able to get it to work

let newGrid = Object.assign({}, this.props.markedFields);

Thanks everyone for helping me out! I am amazed by how friendly everyone is, even though I effectively asked the same thing as countless others before me.

PS: This isn't ideal and I'll work on that, I just wanted to add this reply real quick, in case anyone runs into such a problem outside a reducer as well.

[markerikson]

Alright I feel stupid and sorry for wasting everyone's time, because it's clear that I mutated the state by referencing the original state.
Thanks everyone for helping me out! I am amazed by how friendly everyone is, even though I effectively asked the same thing as countless others before me.

Definitely don't want to give the impression we're attacking anyone for asking a question that's been asked before. That said, it is a very common question, which is why I put it in the FAQ :) In fact, it's pretty much the FAQ-iest of the FAQs, as in, at this point as soon as I see the words "didn't update" or "not rendering", I can 99.9% guarantee it's accidental mutation.

It definitely does take some mental readjustment to get your mind around managing data this way, especially when you're dealing with multiple variables potentially pointing at various levels of a given object structure. It's relatively easy to see the mutation being done if you've only got one level of data, but harder to see if you've got multiple levels and a couple variables involved.

[gaearon]

In general rather than

    let targetID = e.target.id;
    let newGrid = this.props.markedFields;
    newGrid[targetID] = this.props.playerChoice;
    this.props.markField(targetID);

you want to do something like

    let targetID = e.target.id;
    this.props.markField(targetID);

The reducers should take care of producing updated values. Not components.

[gaearon]

For the reference, idiomatic Redux version:

Component

this.props.markField(
  e.target.id,
  this.props.playerChoice
);

Action Creators

export const chooseSide = (side) => ({
  type: 'CHOOSE_SIDE',
  side,
});

export const markField = (targetID, playerChoice) => ({
  type: 'MARK_FIELD',
  targetID,
  playerChoice,
});

Reducers

import { combineReducers } from 'redux';

const playerChoice = (state = null, action) => {
  switch (action.type) {
    case 'CHOOSE_SIDE':
      return action.side;
    default:
      return state;
  }
};

const markedFields = (state = {}, action) {
  switch (action.type) {
    case 'MARK_FIELD':
      return {
        ...state,
        [action.targetID]: action.playerChoice
      };
    default:
      return state;
  }
};

const gameApp = combineReducers({
  playerChoice,
  markedFields,
});

export default gameApp;

[cideM]

Wow thank you so much, here's one month of reddit go.. wait a moment. Well, just thank you very much then <3

[gaearon]

This is correct. The code I suggested is simpler though because it uses reducer composition.

(This referenced a deleted comment which had reducer code updating multiple objects inside a singe case.)

[MiguelCastillo]

@markerikson your response about this question being basically the most FAQ of them all was very graceful. I like pointing those types of things out because they are healthy and awesome for our community and we can't ever have enough of those. :)

I am going to throw in an idea that is absolutely without knowledge of redux internals. So please forgive my ignorance.

The fact this is the most FAQ of them all is a good indicator that perhaps we can take a closer look at handling state updates internally in a slightly different way.

How about returning objects that only contain the changes that the reducer is concerned with? In this case the reducer calls return data that needs to be merged into the final state. Instead of the spread operator, which is the current solution to the issue. I currently use this approach with immutablejs, which makes the state merging workflow really smooth.

Sudo code all the way:

state.withMutations(current => {
//...

var finalstate = fns.reduce((state, reducer) => {
  return state.merge(reducer(state));
}, current);

//...
});

Then all reducers are concerned with is the data they need to update. So

const markedFields = (state = {}, action) {
  switch (action.type) {
    case 'MARK_FIELD':
      return {
        ...state,
        [action.targetID]: action.playerChoice
      };
    default:
      return state;
  }
};

becomes

const markedFields = (state = {}, action) {
  switch (action.type) {
    case 'MARK_FIELD':
      return {
        [action.targetID]: action.playerChoice
      };
    default:
      return; // or empty object... or empty array... or whatever default value.
  }
};

Please educate me if that's completely off topic or not relevant. Maybe this whole thing already works like that anyways... ¯_(ツ)_/¯

[markerikson]

@MiguelCastillo: yeah, certainly as someone who's answered a lot of newbie questions, it's easy to react with "Argh, I've seen that question A GAZILLION TIMES ALREADY!!1!!". But, gotta remember that usually this stuff is new to whoever's asking, and try to find a way to answer the question, point to useful resources, and be encouraging in the process.

Per your reducer suggestion: this actually kinda gets back to what I was saying about the behavior of combineReducers being the "default", over in #1768. If someone wanted to write a reducer wrapper/generator/thingy that called subfunctions, took the "change descriptions", and centralized the merging logic, that's entirely possible.

(erm. bah. accidentally clicked the "reopen" button. Ignore that.)

Anyway, it's generally easier at the moment to assume that whatever subreducer you call is going to be responsible for doing its own updating, and that's what combineReducers assumes. But again, that's just a common utility function, and implementing alternate approaches to managing the internal reducer update process is up to you.

[sompylasar]

@MiguelCastillo The thing is, not every state update is representable as object merge.

Imagine a state that is represented as an array. It requires slice, concat to update its items.

A reducer may even be responsible for a single primitive, non-object value: a number or a boolean.

[MiguelCastillo]

@markerikson I see what #1768 is asking for. But I think what I am suggesting is a tweak to the current behavior. Meaning, that if the reducer is getting a state object passed in as a parameter, and it has to return that same state plus the changes... Why not allow the reducer to only return the changes and have the place where reducers are called from do the merge of the data returned by the reducer into the state?

Similar to setState in react. You don't have to set the whole thing every time... You could pass only what you want to aggregate into the state.

[MiguelCastillo]

@somebody32 sorry - I didn't see your response when I typed my subsequent item...

[gaearon]

Meaning, that if the reducer is getting a state object passed in as a parameter, and it has to return that same state plus the changes... Why not allow the reducer to only return the changes and have the place where reducers are called from do the merge of the data returned by the reducer into the state?

It would break reducer composition. You no longer can safely call child reducers, because any parent reducer would have to take care of merging in exactly the same way. So generic reducer enhancers like https://github.com/omnidan/redux-undo are no longer possible, and it’s hard to use any reducer composition pattern other than combineReducers(). Many apps use custom reducer composition like here and it is hard to make sure you’re always merging objects correctly. Not to say many things aren’t objects: arrays, Immutable.js collections, primitives etc.

[gaearon]

That said you can always make something like

function createMergeReducer(reducer) {
  return (state, action) {
    return { ...state, reducer(state, action) }
  }
}

and use it to generate reducers that automatically merge updates.

[MiguelCastillo]

Sounds good! Thanks for explaining the implications of such change. I like createMergeReducer. That might be a handy tool for others.

[markerikson]

To follow up on this: https://github.com/matthewmueller/socrates appears to implement a reducer abstraction layer that does exactly this kind of "return a diff to merge" approach rather than the typical "return the new object" approach.

[gaearon]

I almost like it, but then there’s coupling of reducers to action types. :-(

[markerikson]

Yeah. Yet another entry for the "Variations" page in my addons catalog: https://github.com/markerikson/redux-ecosystem-links/blob/master/variations.md. (Which is how I'm categorizing stuff that's built on Redux, but carries it in a non-standard-ish direction)