-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Rule proposal: no-access-state-in-setstate #1374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This rule should prevent usage of this.state inside setState calls. Such usage of this.state might result in errors when two state calls is called in batch and thus referencing old state and not the current state. An example can be an increment function: function increment() { this.setState({value: this.state.value + 1}); } If these two setState operations is grouped together in a batch it will look be something like the following, given that value is 1. setState({value: 1 + 1}) setState({value: 1 + 1}) This can be avoided with using callbacks which takes the previous state as first argument. Then react will call the argument with the correct and updated state, even when things happen in batches. And the example above will be something like. setState({value: 1 + 1}) setState({value: 2 + 1})
|
||
``` | ||
function increment() { | ||
this.setState(prevState => ({value: prevState.value + 1})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good advice when the semantic is "increment", but referencing this.state inside a setState call doesn't necessarily mean that the dev doesn't want s "snapshot".
What happens with:
const { value } = this.state;
this.setState({ value: value + 1 });
?
This is conceptually the same code, but it might be a useful way for the author to indicate that this is an intentional ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, I will report this as a warning. The same with
getstate() {
return { value: this.state.counter++ };
}
increase() {
this.setState(getState());
}
I could easily make it only report this.state
directly inside this.setState
, but I don't see why you would use this.state
and not the prevState
-solution which is guaranteed to be up-to-date.
Do you have an example when you actually would want to do that?
This is from the react docs:
If the next state depends on the previous state, we recommend using the updater function form, instead
this.setState((prevState) => { return {counter: prevState.quantity + 1}; });
OMG can this please be merged soon? |
I want this 🙏 |
# Prevent using this.state within a this.setState (no-access-state-in-setstate) | ||
|
||
This rule should prevent usage of this.state inside setState calls. | ||
Such usage of this.state might result in errors when two state calls is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`this.state`
maybe? (with code formatting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can probably do that for all this.state
and setState
mentions in the docs. Thanks, I will fix that first thing to the morning tomorrow.
919fc6c
to
df92a49
Compare
For anyone that needs this rule, I've just released it as a separate plugin for eslint here since this hasn't been going anywhere for a few months. I really hope it can get merged to this project some time, so if the maintainers has some questions, please reach out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code is good, but i'm still not convinced this is an actual problem, despite React's docs' warning.
What do you guys think about the case, where
|
it should, because you are still referencing a possibly outdated value |
I was thinking that, too. For some reason I could get it triggered only by using |
@np-8 can you file that as a new issue? |
Sure, here you are: #1597 |
This rule prevents the use of
this.state
withinthis.setState
. It is further described indocs/rules/no-access-state-in-setstate.md
in the PR, but to summarize:this.setState
is async, and referring tothis.state
within it may lead to race conditions.This is my first eslint-plugin PR, so I am really happy to get any kind of feedback.
edit:
This is also the recommended way doing it in the react docs: