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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add state-in-constructor rule #1945

Open
wants to merge 9 commits into
base: master
from

Conversation

5 participants
@lukyth
Copy link

lukyth commented Aug 18, 2018

This will resolve #1810. Comments are very welcomed. 馃槂

lukyth added some commits Aug 12, 2018

AssignmentExpression(node) {
if (
option === 'never' &&
node.left.object.type === 'ThisExpression' &&

This comment has been minimized.

@alexzherdev

alexzherdev Aug 18, 2018

Contributor

These two lines assume that node.left is a MemberExpression which is not always the case in an AssignmentExpression.

This comment has been minimized.

@ljharb

ljharb Aug 18, 2018

Collaborator

Let's try to add a test case that would fail here.

option === 'never' &&
node.left.object.type === 'ThisExpression' &&
node.left.property.name === 'state' &&
node.parent.parent.parent.parent.kind === 'constructor' &&

This comment has been minimized.

@alexzherdev

alexzherdev Aug 18, 2018

Contributor

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)

This comment has been minimized.

@ljharb

ljharb Aug 18, 2018

Collaborator

Let's try to add a test case that would fail here.

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)

This comment has been minimized.

@alexzherdev

alexzherdev Aug 18, 2018

Contributor

Same here, but in this case maybe utils.getParentES6Component would work

@ljharb
Copy link
Collaborator

ljharb left a comment

Please add more test cases, including covering those mentioned by existing comments.

AssignmentExpression(node) {
if (
option === 'never' &&
node.left.object.type === 'ThisExpression' &&

This comment has been minimized.

@ljharb

ljharb Aug 18, 2018

Collaborator

Let's try to add a test case that would fail here.

option === 'never' &&
node.left.object.type === 'ThisExpression' &&
node.left.property.name === 'state' &&
node.parent.parent.parent.parent.kind === 'constructor' &&

This comment has been minimized.

@ljharb

ljharb Aug 18, 2018

Collaborator

Let's try to add a test case that would fail here.

lukyth added some commits Aug 19, 2018

Remove test cases for `always` option in state-in-constructor rule
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.
Fix a case where AssignmentExpression is deeply nested
This commit also refactor `inConstructor` function from `prop-types`
rule into `utils/Components` so that another rule could use that logic
as well.
Fix a case where left side of assignment is not MemberExpression
This commit also move `isStateMemberExpression` from
`no-direct-mutation-state` rule into `util/Components` so that the logic
could be share with other rules.
@lukyth

This comment has been minimized.

Copy link

lukyth commented Aug 19, 2018

@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 lukyth force-pushed the lukyth:lukyth/state-in-constructor branch from 9063d33 to b0e4c84 Oct 3, 2018

@krawaller

This comment has been minimized.

Copy link

krawaller commented Dec 7, 2018

Just to inject some energy, this looks sweet! Good work @lukyth! :)

@lukyth

This comment has been minimized.

Copy link

lukyth commented Dec 19, 2018

Anything I can do to get this merged?

@ljharb ljharb added the new rule label Dec 27, 2018

@ljharb

ljharb approved these changes Dec 27, 2018

@ljharb ljharb requested review from EvHaus and yannickcr Dec 27, 2018

@EvHaus
Copy link
Collaborator

EvHaus left a comment

Code LGTM.

Could you include an update to the REAMDE.md as well with a link to the new doc?

lukyth added some commits Jan 3, 2019

@lukyth

This comment has been minimized.

Copy link

lukyth commented Jan 3, 2019

@EvHaus I've added this rule to the list in README.md (9415814). Sorry it took me so long. I just saw your comment here.

@EvHaus

EvHaus approved these changes Jan 3, 2019

Copy link
Collaborator

EvHaus left a comment

Thanks! Ready to go.

@ljharb

ljharb approved these changes Jan 3, 2019

Copy link
Collaborator

ljharb left a comment

This seems great, thanks!

(I'm going to hold off merging it for awhile while the 7.12 release settles down)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment