[no-unused-state] Add ability to handle arrow function class method

[jackyho112]

For #1363, so that state property usage by object destructuring assignment in a class method defined with an arrow function gets recorded. Examples:

class ArrowFunctionClassMethodTest extends React.Component {
  constructor() {
    this.state = { foo: 0 };
  }

  doSomething = () => {
    const { state: { foo } } = this;

    return foo;
  }

  render() {
    return <SomeComponent />;
  }
}

should not be warned.

Let me know if there is anything I can do to improve this PR. Also, just to confirm, this rule doesn't care about whether the class method is used or not, right?

[ljharb]

i'm not seeing anything that adds to this set?

If it otherwise crashes because this doesn't exist; then let's initialize it in every case.

[jackyho112]

@ljharb

I think the set, classInfo.aliases, will only be added to if the property name gets assigned to a different variable name.

For instance,

class App extends React.Component {
        state = { foo: 0 };

        doSomething = () => {
          const { state: { foo: bar }} = this;

          return bar;
        }

        render() {
          return <SomeComponent />;
        }
      }

will add 'bar' to the set. I have added one more test regarding this.

I also refactored how the code deals with classInfo.aliases according to your feedback. We still need to clear the set after each method definition though because each alias added is local to the class method. Let me know if this looks better.

[ljharb]

rather than storing across files and clearing; we may want to instead assign classInfo.aliases = new Set() at program entry?

[jackyho112]

@ljharb

Based on your suggestion, I have restored the implementation back to using classInfo.aliases = new Set() which is a part of the original code.

However, we still need to do classInfo.aliases = new Set() for every class method definition because the aliases in classInfo.aliases are local to each class method.

Let me know if that's ok. If not, there are alternative but potentially more complicated ways to implement this, such as making classInfo.aliases an object and setting a property for each class method's aliases.

This seems fine to me; I'm deferring to someone more familiar with this code.

[jackyho112]

@ljharb

Thanks for the review! After this is merged, I can take a look at documenting no-unused-state as I saw there is an issue open for it.

[jackyho112]

@jseminck

I think this test is failing on master. Any clue? Maybe this test should report an error in the first place?

[jseminck]

Thanks @jackyho112

This is because of the new ESLint release. They changed some internal process and a case that we didn't handle before will now work.

I can update the case to be valid, but it would only work with the latest ESLint. With older cases it will still fail. Even more: We had to write some work-around to make sure ESLint did not crash but I can't remove that code unless we update the peerDependency for ESLint.

I also commented here: #1375 (comment)

But I think I can start a new PR and we can start/continue a discussion there.

[jseminck]

Created a PR to move that test: #1437

[jackyho112]

@ljharb, @EvHaus, @yannickcr, @lencioni

Decided to adhere to the original implementation instead because it is the safest approach, it is easier to review and if there is a new way of defining a class method in the future, the rule won't break immediately.

Now, this PR just merely adds the support for a class method defined by an arrow function in a way how the original code intends to and does nothing else. I hope that is ok and should at least make this PR more mergable. 😄

[ljharb]

This does seem much simpler. Test changes LGTM; implementation feems fine.

[EvHaus]

LGTM

[jackyho112]

@EvHaus, @ljharb

Thanks for the approvals! 😄