Skip to content
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

Add no-will-update-set-state rule #1139

Merged
merged 3 commits into from Apr 9, 2017

Conversation

trashstack
Copy link
Contributor

Basically just a copy and paste of the no-did-update-set-state rule for componentWillUpdate. This rule would have saved me a fair bit of time tracking down an issue where this pattern had been used by accident so I thought it was worthwhile adding support for.

docs: {
description: 'Prevent usage of setState in componentWillUpdate',
category: 'Best Practices',
recommended: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should actually be in the recommended rules because there isn't a valid use-case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding things to the recommended rules is a breaking change, so it should be in a separate PR regardless.

@@ -0,0 +1,66 @@
/**
* @fileoverview Prevent usage of setState in componentWillUpdate
* @author Yannick Croissant
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't seem right changing the author when I didn't write any of the code.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - is there any way we could extract out the shared code so it doesn't need to be copy/pasted?

@trashstack
Copy link
Contributor Author

@ljharb Thanks for the feedback, see the new commits for my attempt to factor out the duplicate code.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great!

However now (sorry) I think that maybe instead of making yet another rule, we should just make a single rule that takes an object of lifecycle method names to booleans, rather than having a separate rule for each method?

@trashstack
Copy link
Contributor Author

I'm not sure I completely agree with that. To me there is a distinction between the 2 existing rules and the rule I'm adding. The existing rules are something that is normally a bad pattern but can work (and in the rarest of circumstances might even be correct) whereas the new rule catches code that should never exist and is likely buggy. So putting them all under the same rule with an argument might give the wrong impression about the varying levels of strictness with which the rules should be applied.

Perhaps the distinction between between the rules isn't as great as I'm interpreting or the reduction in code is worthwhile enough to do this regardless though. I'm open to being swayed but right now I think I'd advocate to keep them separate.

@ljharb
Copy link
Member

ljharb commented Apr 9, 2017

That seems like a reasonable argument. I'm good with just adding this rule.

@trashstack
Copy link
Contributor Author

Awesome. Thanks very much for the reviews and feedback.

@ljharb ljharb merged commit 0ae6bb2 into jsx-eslint:master Apr 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants