Eslint-plugin-react: sort-comp rule is broken

[nuragic]

Hi folks,

First of all thanks for the great work and effort on this.

I'm opening this issue because I believe that the actual order is broken, consider this example:

class App extends PureComponent {
  something = {
    whatever: true,
  };

  state = {
    count: 0,
  };

  constructor(props) {
    super(props);
    this.ws = new WebSocket("ws://localhost:5000/api/1.0/events");
    this.ws.onerror = this.onWsError.bind(this);
    this.ws.onmessage = this.onWsMessage.bind(this);
  }

  componentDidMount() {
    // ...
  }

  render() {
    // ...
  }

  onWsMessage(ev) {
    // ...
  }

  onWsError(ev) {
    // ...
  }

}

Which produces these errors:

something should be placed after onWsError
state should be placed after constructor

😟 Mhh... ok, let see:

class App extends PureComponent {

  constructor(props) {
    super(props);
    this.ws = new WebSocket("ws://localhost:5000/api/1.0/events");
    this.ws.onerror = this.onWsError.bind(this);
    this.ws.onmessage = this.onWsMessage.bind(this);
  }

  state = {
    count: 0,
  };

  componentDidMount() {
    // ...
  }

  render() {
    // ...
  }

  onWsMessage(ev) {
    // ...
  }

  onWsError(ev) {
    // ...
  }

  something = {
    whatever: true,
  };

}

😄 I hope you agree with me that it doesn't make sense... state is included inside the _lifecycle_ category, but something in the _everything-else_, while in fact both are just public fields which should be both initialised before the constructor IMHO.

That would make sense for you? I could make a PR if you want.

Thanks!

[ljharb]

Hmm, yes - state is a public class field, it's not "lifecycle".

Putting public fields before the constructor - no matter what kind of data they contain - seems reasonable to me. We can't change the defaults without it being semver-major, but we should at least make it possible to change this.

[nuragic]

Yup, unfortunately this one would be a breaking change. Maybe a codemod could also be provided in order to update?

[ljharb]

Sure, but that doesn’t make it nonbreaking.

We could queue it up and just wait until it’s time, but it’d be ideal to find a nonmajor improvement to make now.

[nuragic]

Maybe we could start adding a new (optional for now) group like e.g. class-fields, which would decouple class fields from everything-else; but what about moving state?

I imagine having something like this in my configuration:

{
  order: [
    'static-methods',
    'lifecycle',
    'everything-else',
    'render'
  ],
  groups: {
    'class-fields': [
      'private-fields',
      'public-fields': [
        '*',  // i.e. any public field
        'state', // <<<<< moved here
       ]
    ],
    lifecycle: [
      'displayName',
      'propTypes',
      'contextTypes',
      'childContextTypes',
      'mixins',
      'statics',
      'defaultProps',
      'class-fields' // <<<<< would this be possible right now?
      'constructor',
      'getDefaultProps',
      'getInitialState',
      'getChildContext',
      'getDerivedStateFromProps',
      'componentWillMount',
      'UNSAFE_componentWillMount',
      'componentDidMount',
      'componentWillReceiveProps',
      'UNSAFE_componentWillReceiveProps',
      'shouldComponentUpdate',
      'componentWillUpdate',
      'UNSAFE_componentWillUpdate',
      'getSnapshotBeforeUpdate',
      'componentDidUpdate',
      'componentDidCatch',
      'componentWillUnmount'
    ]
  }
}

As we can see the lifecycle group is misleading cause it's grouping more stuff than React component's lifecycle, but that must be solved in a major release. With the above config, people could decide to override it and put the class-field whenever they want... do you see any flaws? :)

[ljharb]

It's tough to evaluate without a PR, to be honest.

[nuragic]

Ok, I'll try to make a PR then!

[nuragic]

ℹ️ I can't really find the time at the moment for this PR, sorry! Hopefully next week(s)... 😞 Anyway, if someone would like to make an intent in the meantime, it'll be very welcome! 😄 🤗