Add state-in-constructor rule #1945
Conversation
lib/rules/state-in-constructor.js
Outdated
| AssignmentExpression(node) { | ||
| if ( | ||
| option === 'never' && | ||
| node.left.object.type === 'ThisExpression' && |
There was a problem hiding this comment.
These two lines assume that node.left is a MemberExpression which is not always the case in an AssignmentExpression.
There was a problem hiding this comment.
Let's try to add a test case that would fail here.
lib/rules/state-in-constructor.js
Outdated
| option === 'never' && | ||
| node.left.object.type === 'ThisExpression' && | ||
| node.left.property.name === 'state' && | ||
| node.parent.parent.parent.parent.kind === 'constructor' && |
There was a problem hiding this comment.
This is a bit brittle: assignment could be nested in other nodes (e.g. if conditions) in which case this sequence will not work. This should either be traversing parents in a loop, or scopes like https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/no-unused-prop-types.js#L264 (although not sure which one is better)
There was a problem hiding this comment.
Let's try to add a test case that would fail here.
lib/rules/state-in-constructor.js
Outdated
| node.left.object.type === 'ThisExpression' && | ||
| node.left.property.name === 'state' && | ||
| node.parent.parent.parent.parent.kind === 'constructor' && | ||
| utils.isES6Component(node.parent.parent.parent.parent.parent.parent) |
There was a problem hiding this comment.
Same here, but in this case maybe utils.getParentES6Component would work
lib/rules/state-in-constructor.js
Outdated
| AssignmentExpression(node) { | ||
| if ( | ||
| option === 'never' && | ||
| node.left.object.type === 'ThisExpression' && |
There was a problem hiding this comment.
Let's try to add a test case that would fail here.
lib/rules/state-in-constructor.js
Outdated
| option === 'never' && | ||
| node.left.object.type === 'ThisExpression' && | ||
| node.left.property.name === 'state' && | ||
| node.parent.parent.parent.parent.kind === 'constructor' && |
There was a problem hiding this comment.
Let's try to add a test case that would fail here.
Since `always` is the default option, we can use the no-option cases to cover these `always` cases. I left one `always` case to make sure that having an option as `always` will act the same way as no-option.
This commit also refactor `inConstructor` function from `prop-types` rule into `utils/Components` so that another rule could use that logic as well.
This commit also move `isStateMemberExpression` from `no-direct-mutation-state` rule into `util/Components` so that the logic could be share with other rules.
|
@alexzherdev @ljharb Thank you so much for your reviews! I've addressed all of them with these commits 81d0e8b...b1024c0. Could you please check them out? I also removed some test cases which I think are redundant (81d0e8b). I'm not sure if that's the right call. If you guys don't think so then I can bring them back. |
lukyth
force-pushed
the
lukyth/state-in-constructor
branch
from
9063d33
to
b0e4c84
Compare
|
Just to inject some energy, this looks sweet! Good work @lukyth! :) |
|
Anything I can do to get this merged? |
This will resolve #1810. Comments are very welcomed. 馃槂