Eslint-plugin-react: Rule idea: Disallow accessing this.state within setState

[relekang]

When using previous state in setState it can be unsafe to use this.state, because the scheduling and batching in react might result in this.state being something else than when the function was called. This might lead to race condition like a counter being set to 1, 2, 3, 3 if the two last calls are called within a batch when the expected result would be 1, 2, 3, 4.

Example

// Good
this.setState((previousState) => ({ counter: previousState.counter + 1 }));

// Bad
this.setState({counter: this.state.counter + 1});

I am currently working on implementing a rule like that enforces using callback pattern if state is accessed within setState. Is this something that you would like to have in this plugin?

React docs on this topic

[lencioni]

Yes, I think this would be great to add. It even seems like it would be autofixable.

[ljharb]

In the latter case, the value of this.state.counter is stored by reference in the object passed to this.setState - I don't see the problem. The batching would grab the momentary value of all the counter values, and it'd remain correct.

If you passed a function to setState (which I wasn't aware is possible; I've only ever seen an object passed as the first argument) then certainly that function accessing anything on this besides props is dangerous, but that doesn't seem to be what you're warning for here.

Can you elaborate?

[relekang]

I should have added this link in the description, but forgot. Anyways, the React docs states that the latter case might end up in erroneous state changes. I don't know enough about the internals of React to say something about the probability of the error happening.

[ljharb]

I agree that the rule you're requesting is in line with the React docs, however it really seems to me that the React docs are incorrect, as it should be impossible for batching of updates to affect this behavior.

@spicyj @zpao @sebmarkbage et al: can you help me understand why batching updates would make the example in https://facebook.github.io/react/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous observably different, given that the value of the "counter" key is reified synchronously and instantaneously before it's passed into setState, thus this.state and this.props are no longer linked to the values by the time setState is batched?

[sebmarkbage]

this.state doesn't update until the system has fully flushed. In a batch you may have two of these. The callback model executes while the system in flushing, so the argument passed in is later in time.

Currently, the only way this happens is if you fire custom events within the same event tick. Such as Flux stores, or multiple callbacks firing. However, in Fiber, this can happen is many more cases because a single batch can last across several events and even several frames.

[ljharb]

Gotcha - sounds like it's not a common issue now, but might become one later, so this would fall under a "future-facing best practice" bucket.

Thanks for explaining!

[adnasa]

I'm up for implementing this

[relekang]

@adnasa Cool, I have something "working" in a branch. Not sure if it helps or not. I don't think that implementation is good enough, but might be worth take a look.

[adnasa]

I'll have a look, thanks @relekang

[kaiyoma]

Is this rule still being implemented? It's listed in the README as being a "supported rule", but when I try to use it, even with the latest code, I get errors like this:

1:1  error  Definition for rule 'react/no-access-state-in-setstate' was not found  react/no-access-state-in-setstate

If it's not officially supported yet, maybe it shouldn't be in the README?

[jomasti]

@kaiyoma There hasn't been a release with the rule, and the README reflects master. This is the README for the latest release (7.4.0) where the rule is not listed.

[kaiyoma]

Great, thanks for clarifying. Looking forward to having this rule as it's one less thing I need to be on the lookout for during code reviews. 😄

[ljharb]

Closed in #1374