sort-comp: Consider separating static fields & instance fields into separate groups

[shangxiao]

Currently sort-comp asks me to place instance fields after methods in my class. It's conventional to place instance fields above the constructor, underneath static fields.

Possibly also consider allowing static fields be grouped separately from static methods.

[ljharb]

I'd expect static fields and static methods to both be the last items in the class body, after everything instance-related is done - although I agree that instance fields could belong above the constructor.

[le0nik]

I tried to add this feature with this PR: #685.

Will gladly take suggestions and feedback on it.

[yannickcr]

Also, to continue in this direction, it could be good to have separate groups for static-method, static-properties, instance-methods and instance-properties.

[le0nik]

@yannickcr I like it, but it raises a couple of questions:

1. What would be considered instance-methods and static-methods? Would they be just arrow function properties or also regular functions? I'll illustrate with examples.

Statics fields:

class Hello extends React.Component {
  static displayName = 'Hello'; // static-properties
  static fetch() {} // static-methods
  static process = () => {} // static-methods ?
}

Instance fields:

class Hello extends React.Component {
  count = 0; // instance-properties
  handleClick = () => {} //  instance-methods
  renderSeciton() {} // instance-methods ?
}

I think it should be consistent for both static-methods and instance-methods methods, but then renderSection in the example above would be semantically incorrect, because it wouldn't be an instance method.

2. Is it possible to combine methods and properties in configuration to allow users to mix them up by default?

Right now static-methods allows this:

class Hello extends React.Component {
  static prop1 = 'Hello';
  static fetch() {}
  static prop2 = 'Bye';
}

but configuration like this:

[
  'static-properties',
  'static-methods'
]

wouldn't allow this and users wouldn't be able to go back to the way it was before without applying codemod

Thoughts?

[ljharb]

There's actually a difference between a class method and a function-valued property - handleClick = () => {} is not an instance method, it's an instance property that has a function on it.

[le0nik]

@ljharb I think that it's pretty much the same thing, because functions on the prototype can be considered a property too. It's obvious if you use ES5 syntax:

class Hello extends React.Component {
  renderSection: function() {
  }
}

In my opinion we need to determine what instance actually is. I thought of it as an object itself as opposed to it's prototype and wanted to separate instance functions(which are basically function-valued properties, like handleClick = () => {}) and instance properties which users usually use to store data. The reason for this is that the most common case for function-valued properties on an instance(like handleClick) is the binding to this value(usually for event handlers), so users don't usually replace or assign to those properties later on. So

instance-methods would be things like handleClick = () => {};
and
instance-properties would be things like count = 0;

Maybe there can also be a prototype-methods special keyword, that would allow users to determine how functions like handleClick = () => {} and renderSection() {} should be ordered.

What do you think?

[ljharb]

The difference is that class methods are non-enumerable (properties are enumerable) and they have a [[HomeObject]], which means they're eligible to use super.

[le0nik]

This is a good point. Didn't look at it from this perspective.

For now I don't know how to move further with this. There are multiple approaches that could be taken.
Could someone else weigh in on it?

[jozanza]

This may be outside of the scope of this particular PR, but I would love to have more granular control over the ordering without the use of groups. Similar to how regular expressions may be used inside of the order property, it would be really nice to have a syntax that disambiguates static, prototype, instance props/methods. At the moment, this linting rule will not play nice with classes that give the same name to both a static and a prototype/instance property due to this ambiguity.

Perhaps something along these lines:

// .eslintrc
"react/sort-comp": [2, {
  "order": [
    "foo", // prototype method (default -- already supported)
    "/bar/", // prototype method regex (already supported)
    "static foo", // static initializer
    "static /bar/", // static initializer regex
    "foo=", // instance initializer
    "/bar/=" // instance initializer regex
  ]
}]

My apologies if this wasn't quite what was meant by "weighing in". Thoughts?

^{Sidenote: Another thought I've had is that this plugin has utility beyond React Component classes. There's not much keeping it from being generally useful for all ES6+ class declarations. If the ambiguities between static/prototype/instance are tackled, that only leaves a few more cases such as getter/setters and symbols. I'd be happy to create a new issue for discussion if this isn't the appropriate thread.}

[ktaras]

Strange. It seems like instance-variables works only for state.
I specified following order:

      - static-methods
      - instance-variables
      - lifecycle
      - everything-else
      - render

When I declare

class CoolComponent extends Component {
  state = {};
  ...
}

it's OK, but when I add some custom property, e.g.:

class CoolComponent extends Component {
  state = {};
  someList = [];
  ...
}

it throws an error:

someList should be placed after componentWillUnmount

[ljharb]

@ktaras thanks, could you file that as a separate issue?