sort-comp wrong order with RegExp rules and static methods

[nosilleg]

I believe this issues comes from #1795

The Airbnb rules for react include sort-comp with regex matching. source here

If you have a static method that matches any of the 3 RegExp values (e.g. renderCell) then this will push to indexes, and the following check for method.static will not happen because of the new conditional on empty indexes.

This should be valid according to the Airbnb Rules.

class Row extends React.Component {
  static getDelta(data) {
    ...
  }

  static onTimeout() {
    ...
  }

  static renderCell(data) {
    ...
  }

  constructor(props) {
    ...
  }

  render() {
    ...
  }
}

[ljharb]

Is the issue that #1795 needs to be reverted, or that the regex checking needs to be done in a second place?

[lynxtaa]

I think we should check for getDerivedStateFromProps more explicitly rather than use indexes.length === 0 condition

[ljharb]

that makes sense. Can someone put up a PR?

[nosilleg]

I'm creating a PR. The fix is a bit more involved than a revert or added RegExp check.

[nosilleg]

The documentation isn't clear on expected behaviour. Does static trump all else? If something matches a RegExp and is static, does it go in the RegExp or static group? Does the order of rules impact this? (If RegExp is first, then it's RegExp, and if static is first then it's static?)

[nosilleg]

If static-methods isn't included, then should it match the RegExp or everything-else?

[lynxtaa]

There's a contradiction when method is static and matches regexp:

• if static is more important, than we can't use regexps for matching static methods (which is not good)
• if regexp is more important, than current behaviour is not a bug

I think the problem is more deeper than it seems.

[nosilleg]

The way that I'm implementing it at the moment is group order based. It's not a massive change to make statics the priority, but as you say, the API isn't flexible enough to cover all potential desired behaviours.

[nosilleg]

Possible change of plan. I just hit the test that says that if a method matches two RegExps then it can be placed in either location.

Should this be the case for everything?

That would mean that static getDerivedStateFromProps could either he with the lifecycle methods or with the statics.

The example in the issue description could have the methods with the statics, or with the RegExps.

This is the most flexible option, but there will be people who do not like that flexibility.

[lynxtaa]

I think by default we should place getDerivedStateFromProps within lifecycle methods. But users should be able to override this behaviour by excluding getDerivedStateFromProps from lifecycle group.

For example, in this case lifecycle group doesn't include getDerivedStateFromProps, so it should go with statics.

[nosilleg]

I have no doubt that havinggDDFP in lifecycle is a valid view. I'm willing to bet others think having it with statics is preferable. The precedence set with RegExps when this rule was first created is that things can appear in any matching group.

Making it more strict would require going back to your original PR commit and having explicit exceptions to rules. It may be the case that my PR will be a partial revert of your PR in your eyes, since it's less strict.

Allowing static methods/properties to be in multiple places fixes two additional issues. #1838 and #1808

I'll push the PR and see what the feedback is. It should be done shortly after adding a test for an undiscovered bug that my changes fix. (While hopefully not breaking things we don't have tests for.)