Ignore class properties in destructuring-assignment

[alexandernanberg]

Fixes #1875

Please let me know if you would like me to add more tests or if anything can be improved

[ljharb]

This should be behind an option; the default behavior should be to warn inside class properties.

[alexandernanberg]

Hmm not sure that I agree with you on that, because there is no way to "silence" the warning without refactor the code to use a constructor instead, which in my opinion is unnecessary.

But I'll update my PR accordingly!

[ljharb]

That's one of the points of the rule - if you need to rely on props values, it's supposed to force it to go in the constructor.

[alexandernanberg]

Okay I get it!

How would you like the option to be implemented? Not sure that I see a clear way to do it well without a breaking change. I guess we could add another enum option like ignoreClassProperties but I feel that it would maybe be a bit ambiguous?

[ljharb]

allowInClassFields seems reasonable to me, or allow: ["class-fields"] (where that's the only allowed value atm)

[alexandernanberg]

Okay so I've now added the option, I went for another name because I thought it was more logical than using the "allow" word, but let me know if you want me to change it. I also a bit unsure if I wrote the schema correctly

So you would do this;
["error", "always", { "ignoreClassFields": true }]

[ljharb]

when set to “never”, this should still apply - you could maybe have foo = function () { const { a } = this.props; }.call(this), or when do expressions land, you could have foo = do { const { a } = this.props; a; }

[alexandernanberg]

Makes sense, I updated the PR so it should work with "never" as well 👍

[ljharb]

shouldn't https://github.com/yannickcr/eslint-plugin-react/pull/1909/files#diff-d73ce54079fb0715f60db27176a09fb6R135 also be updated?

[alexandernanberg]

Yep it probably should. Sorry for missing that, I've never worked in AST or eslint-plugin projects before so it's all new to me. I've updated the PR again.

[HenriBeck]

@ljharb should this be behind a flag because it doesn't have to do anything with destructuring assignments? I would agree that it would be a great rule to disallow props usage inside class properties but doing it here might be a little bit confusing for people.

[ljharb]

@HenriBeck the rule is to force, or forbid, destructuring assignment regardless of location inside a class body. That class fields make destructuring awkward might be an inconvenience to some, hence the need for an option - but personally, I like the side effect that if you're trying to do anything complex in a class field, it forces you to properly put it in a constructor.

[HenriBeck]

@ljharb I agree with you that doing complicated things inside class properties isn't the best idea.
But forbidding it in this rule seems odd because class properties aren't about destructuring and should be able to disallow them whether or not you have this rule configured.

[ljharb]

Methods aren't about destructuring either - the rule deals with any references to state, props, or context, no matter where they appear.

[alexandernanberg]

I think this should be ready to get merged unless there is anything else you want me to adjust?

[alexandernanberg]

Well shit, this does not work when assigning an object with properties containing this.props to a class field.

class Foo extends React.Component {
  state = {
    // this will still throw an error
    bar: this.props.bar
  }
}

Did a quick try to fix this but didn't make any progress, can somebody either try to solve this or point me in the right direction? @ljharb

[techimist]

Yup 7.11.1, the below still warns me about de-structuring props assignment:
const sampleSchemaData = this.props.schemaData

with the following rule in eslintrc:

"rules": {
    "react/destructuring-assignment": [1, "always", { "ignoreClassFields": true }]
}

[ljharb]

@Anna93 that’s a variable, not a class field.

[zslabs]

This still throws one for me using the same setup as @Anna93:

class Input extends React.Component {
  id = `${this.props.name}-${generateUUID()}`;
 ...