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

No set state in constructor rule #2384

Open
wants to merge 5 commits into
base: master
from

Conversation

@jenil94
Copy link
Contributor

commented Aug 17, 2019

Fixes #2371

Copy link
Collaborator

left a comment

This seems like it’d be better as an option to no-set-state rather than a separate rule.

README.md Outdated
@@ -121,7 +121,8 @@ Enable the rules that you would like to use.
* [react/no-multi-comp](docs/rules/no-multi-comp.md): Prevent multiple component definition per file
* [react/no-redundant-should-component-update](docs/rules/no-redundant-should-component-update.md): Prevent usage of `shouldComponentUpdate` when extending React.PureComponent
* [react/no-render-return-value](docs/rules/no-render-return-value.md): Prevent usage of the return value of `React.render`
* [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`
* [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`,

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 17, 2019

Collaborator
Suggested change
* [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`,
* [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`
@jenil94

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@ljharb How about adding ignoreExceptInConstructor in no-set-state? If set to true it will consider this.setState as error only in constructor.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2019

@jenil94 maybe something a bit more scalable would be an "in" option that took either the string "all" or an array of strings, which at the moment only can include "constructor"?

@jenil94 jenil94 force-pushed the jenil94:no-set-state-in-constructor branch from be577fc to 82d5979 Sep 19, 2019
@@ -29,22 +44,49 @@ module.exports = {
* @param {Object} component The component to process
* @returns {Boolean} True if the component is valid, false if not.
*/

const configuration = context.options[0] || {};
const notAllowedIn = configuration.in;

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 1, 2019

Collaborator
Suggested change
const notAllowedIn = configuration.in;
const notAllowedIn = new Set(configuration.in);
node: setStateUsage,
message: 'Do not use setState'
});
if (notAllowedIn && notAllowedIn.length && component.setStateUsages.length) {

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 1, 2019

Collaborator

let's just delete the else case entirely, and unconditionally use the if case.

if (notAllowedIn && notAllowedIn.length && component.setStateUsages.length) {
component.setStateUsages.filter((node) => {
const calleName = getMethodName(node);
return notAllowedIn.filter(notAllowed => notAllowed === calleName).length;

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 1, 2019

Collaborator
Suggested change
return notAllowedIn.filter(notAllowed => notAllowed === calleName).length;
return notAllowedIn.filter(notAllowed => notAllowed === calleeName).length;
jenil94 added 2 commits Oct 12, 2019
@jenil94 jenil94 marked this pull request as ready for review Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.