-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Rule proposal: no-access-state-in-setstate #1374
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# 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 | ||
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: | ||
|
||
``` | ||
function increment() { | ||
this.setState(prevState => ({value: prevState.value + 1})); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 Do you have an example when you actually would want to do that? This is from the react docs:
|
||
} | ||
``` | ||
|
||
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}) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
/** | ||
* @fileoverview Prevent usage of this.state within setState | ||
* @author Rolf Erik Lekang, Jørgen Aaberg | ||
*/ | ||
|
||
'use strict'; | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Rule Definition | ||
// ------------------------------------------------------------------------------ | ||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: 'Reports when this.state is accessed within setState', | ||
category: 'Possible Errors', | ||
recommended: false | ||
} | ||
}, | ||
|
||
create: function(context) { | ||
function isSetStateCall(node) { | ||
return node.type === 'CallExpression' && | ||
node.callee.property && | ||
node.callee.property.name === 'setState' && | ||
node.callee.object.type === 'ThisExpression'; | ||
} | ||
|
||
// The methods array contains all methods or functions that are using this.state | ||
// or that are calling another method or function using this.state | ||
const methods = []; | ||
// The vars array contains all variables that contains this.state | ||
const vars = []; | ||
return { | ||
CallExpression(node) { | ||
// Appends all the methods that are calling another | ||
// method containg this.state to the methods array | ||
methods.map(method => { | ||
if (node.callee.name === method.methodName) { | ||
let current = node.parent; | ||
while (current.type !== 'Program') { | ||
if (current.type === 'MethodDefinition') { | ||
methods.push({ | ||
methodName: current.key.name, | ||
node: method.node | ||
}); | ||
break; | ||
} | ||
current = current.parent; | ||
} | ||
} | ||
}); | ||
|
||
// Finding all CallExpressions that is inside a setState | ||
// to further check if they contains this.state | ||
let current = node.parent; | ||
while (current.type !== 'Program') { | ||
if (isSetStateCall(current)) { | ||
const methodName = node.callee.name; | ||
methods.map(method => { | ||
if (method.methodName === methodName) { | ||
context.report( | ||
method.node, | ||
'Use callback in setState when referencing the previous state.' | ||
); | ||
} | ||
}); | ||
|
||
break; | ||
} | ||
current = current.parent; | ||
} | ||
}, | ||
|
||
MemberExpression(node) { | ||
if ( | ||
node.property.name === 'state' && | ||
node.object.type === 'ThisExpression' | ||
) { | ||
let current = node; | ||
while (current.type !== 'Program') { | ||
// Reporting if this.state is directly within this.setState | ||
if (isSetStateCall(current)) { | ||
context.report( | ||
node, | ||
'Use callback in setState when referencing the previous state.' | ||
); | ||
break; | ||
} | ||
|
||
// Storing all functions and methods that contains this.state | ||
if (current.type === 'MethodDefinition') { | ||
methods.push({ | ||
methodName: current.key.name, | ||
node: node | ||
}); | ||
break; | ||
} else if (current.type === 'FunctionExpression') { | ||
methods.push({ | ||
methodName: current.parent.key.name, | ||
node: node | ||
}); | ||
break; | ||
} | ||
|
||
// Storing all variables containg this.state | ||
if (current.type === 'VariableDeclarator') { | ||
vars.push({ | ||
node: node, | ||
scope: context.getScope() | ||
}); | ||
break; | ||
} | ||
|
||
current = current.parent; | ||
} | ||
} | ||
}, | ||
|
||
Identifier(node) { | ||
// Checks if the identifier is a variable within an object | ||
let current = node; | ||
while (current.parent.type === 'BinaryExpression') { | ||
current = current.parent; | ||
} | ||
if (current.parent.value === current) { | ||
while (current.type !== 'Program') { | ||
if (isSetStateCall(current)) { | ||
vars | ||
.filter(v => v.scope === context.getScope()) | ||
.map(v => context.report( | ||
v.node, | ||
'Use callback in setState when referencing the previous state.' | ||
)); | ||
} | ||
current = current.parent; | ||
} | ||
} | ||
} | ||
}; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
/** | ||
* @fileoverview Prevent usage of this.state within setState | ||
* @author Rolf Erik Lekang, Jørgen Aaberg | ||
*/ | ||
'use strict'; | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Requirements | ||
// ------------------------------------------------------------------------------ | ||
|
||
const rule = require('../../../lib/rules/no-access-state-in-setstate'); | ||
const RuleTester = require('eslint').RuleTester; | ||
|
||
const parserOptions = { | ||
ecmaVersion: 6, | ||
ecmaFeatures: { | ||
jsx: true | ||
} | ||
}; | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Tests | ||
// ------------------------------------------------------------------------------ | ||
|
||
const ruleTester = new RuleTester(); | ||
ruleTester.run('no-access-state-in-setstate', rule, { | ||
valid: [{ | ||
code: [ | ||
'var Hello = React.createClass({', | ||
' onClick: function() {', | ||
' this.setState(state => ({value: state.value + 1}))', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions | ||
}, { | ||
code: [ | ||
'var Hello = React.createClass({', | ||
' multiplyValue: function(obj) {', | ||
' return obj.value*2', | ||
' },', | ||
' onClick: function() {', | ||
' var value = this.state.value', | ||
' this.multiplyValue({ value: value })', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions | ||
}], | ||
|
||
invalid: [{ | ||
code: [ | ||
'var Hello = React.createClass({', | ||
' onClick: function() {', | ||
' this.setState({value: this.state.value + 1})', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions, | ||
errors: [{ | ||
message: 'Use callback in setState when referencing the previous state.' | ||
}] | ||
}, { | ||
code: [ | ||
'var Hello = React.createClass({', | ||
' onClick: function() {', | ||
' this.setState(() => ({value: this.state.value + 1}))', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions, | ||
errors: [{ | ||
message: 'Use callback in setState when referencing the previous state.' | ||
}] | ||
}, { | ||
code: [ | ||
'var Hello = React.createClass({', | ||
' onClick: function() {', | ||
' var nextValue = this.state.value + 1', | ||
' this.setState({value: nextValue})', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions, | ||
errors: [{ | ||
message: 'Use callback in setState when referencing the previous state.' | ||
}] | ||
}, { | ||
code: [ | ||
'function nextState(state) {', | ||
' return {value: state.value + 1}', | ||
'}', | ||
'var Hello = React.createClass({', | ||
' onClick: function() {', | ||
' this.setState(nextState(this.state))', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions, | ||
errors: [{ | ||
message: 'Use callback in setState when referencing the previous state.' | ||
}] | ||
}, { | ||
code: [ | ||
'var Hello = React.createClass({', | ||
' nextState: function() {', | ||
' return {value: this.state.value + 1}', | ||
' },', | ||
' onClick: function() {', | ||
' this.setState(nextState())', | ||
' }', | ||
'});' | ||
].join('\n'), | ||
parserOptions: parserOptions, | ||
errors: [{ | ||
message: 'Use callback in setState when referencing the previous state.' | ||
}] | ||
}] | ||
}); |
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
andsetState
mentions in the docs. Thanks, I will fix that first thing to the morning tomorrow.