Discussion: Cloning before setting state

[eek]

I've had this problem multiple times before, I don't know if I should be more careful or to actually fix it in the core.

The problem is, by getting the value from a store, like:

const currentState = this.state;
// then modifying a bit from it.
currentState.toggled = !currentState.toggled;
// then setting it back.
this.setState(currentState);

the problem is by having the whole object into currentState, and modifying it, also modifies in the current StoreState via object reference.

so when I then try to see if a Component that listens to a store should update.

shouldComponentUpdate(nextProps, nextState) {
    // check the nextState with the current one
    return nextState.toggled !== this.state.toggled;
}

nextState and this.state.toggled is actually the same state. I know this can be avoided by only setting in the Store:

this.SetState({ toggled: !this.state.toggled });

But I'm thinking that more people would have this issue (especially when dealing with multi-level objects), wouldn't it be better for cloning the state before setting it?

Like changing in defineReact.js when

sS(updateObj); to sS(clone(updateObj));
and this.setState(updateObj); to this.setState(clone(updateObj));

This will definitely prevent all reference issues and it will be the same as Reflux.getGlobalState

What do you guys think?

[BryanGrezeszak]

Mutating state directly is definitely improper practice. Not just here, but even in React itself it's called out as something not do do. You could very well get the same issue if you tried to do that on a normal React component's state. From the React docs:

Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were immutable.

The proper way to do it is the way you described later, something like:

this.setState({toggled: !this.state.toggled});

And not just for the reason of avoiding directly mutating state either. In your first example if state had more properties than just toggled we'd be setting all of them in the setState, even though we only need to update one. That can lead to unnecessary rendering too. That's not as big of a deal in normal react (because if you're changing 1 piece of state then the component usually has to re-render anyway), but in a flux environment where multiple components may utilize different parts of the state you could re-render a dozen components when only one of them actually needed it.

But lastly: I'm also fairly sure cloning would not solve this. You mutated the state before setState, meaning before the proposed cloning would have happened. So it would just be cloning your mutated state object.

[FlorianWendelborn]

@BryanGrezeszak can we check if the last and next state are the same? Then we can show a warning or something like that.

[BryanGrezeszak]

@dodekeract - If you mean for part of Reflux detect whether the state has been mutated directly: unfortunately no.

That's actually the whole reason for setState (both here and in React). If you want state-based applications with simple object-based state (which is what React does) then you have to at least have some sort of function call to mutate the state and only mutate it that way (never directly), because there's no good/efficient way in JS to detect direct object mutations.

I mean, you could clone/check repeatedly, but that would dodgy, and at best only be useful enough for warnings and would probably add about as much overhead as the rest of the running library's features combined (which is not a good feature vs. cost ratio at all).

And, on top of that, even if we did that in Reflux...React doesn't. So it makes more sense to just piggyback off React and say "you can't mutate it directly in React, so don't mutate it directly here."

Honestly, if there were an efficient and practical way to detect direct state mutation of simple objects then both React and Reflux wouldn't use something like setState at all, we'd just let you mutate away on the objects and update the app to your state as you go. Unfortunately JS doesn't really work that way.