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

ManThursday
Copy link
Contributor

@ManThursday ManThursday commented Apr 5, 2017

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

@ManThursday ManThursday Apr 5, 2017

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

@ljharb ljharb Apr 5, 2017

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

@ManThursday ManThursday Apr 5, 2017

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.

ljharb
ljharb approved these changes Apr 5, 2017
Copy link
Member

@ljharb ljharb left a comment

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

@ljharb ljharb added the new rule label Apr 5, 2017
@ljharb ljharb requested review from yannickcr, lencioni and EvHaus Apr 5, 2017
@ManThursday
Copy link
Contributor Author

ManThursday commented Apr 6, 2017

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

ljharb
ljharb approved these changes Apr 6, 2017
Copy link
Member

@ljharb ljharb left a comment

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?

@ManThursday
Copy link
Contributor Author

ManThursday commented Apr 9, 2017

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.

@ManThursday
Copy link
Contributor Author

ManThursday commented Apr 9, 2017

Awesome. Thanks very much for the reviews and feedback.

@ljharb ljharb merged commit 0ae6bb2 into jsx-eslint:master Apr 9, 2017
3 checks passed
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