[sort-comp] Fix bug affecting `instance-methods` group

[michaelhogg]

Many thanks to @yannickcr and all contributors for developing this great plugin :slightly_smiling_face:

Regarding issue #599 ("sort-comp: Consider separating static fields & instance fields into separate groups"), and PR #1470 ("Add instance-methods and instance-variables to sort-comp") released in v7.6.0, there's a bug in the implementation of the instance-methods group :bug:

Class instance methods can be declared in two different ways (AST Explorer snippet):

class Main extends React.Component {

    // MethodDefinition -> FunctionExpression
    foo() {}

    // ClassProperty -> ArrowFunctionExpression
    bar = () => {}

}

Using the babel-eslint parser, if a class instance method is declared as a FunctionExpression, then the parent AST node is a MethodDefinition, but the sort-comp rule is expecting the parent node to be a ClassProperty, which is wrong :x:

---

Unfortunately the unit tests didn't detect this bug.  Here's the source code for the valid unit test, which I've annotated with comments showing the AST nodes and the relevant propertiesInfos (AST Explorer snippet):

class Hello extends React.Component {

    // ClassProperty -> ArrowFunctionExpression (instanceMethod == true)
    foo = () => {}

    // MethodDefinition -> FunctionExpression (instanceMethod == false)
    constructor() {}

    // MethodDefinition -> FunctionExpression (instanceMethod == false)
    classMethod() {}

    // ClassProperty -> ArrowFunctionExpression (static == true, instanceMethod == false)
    static bar = () => {}

    // MethodDefinition -> FunctionExpression (instanceMethod == false)
    render() {
        return <div>{this.props.text}</div>;
    }

}

For the constructor(), classMethod() and render() methods, instanceMethod is false but should be true :x:

The group order for that unit test is:

• instance-methods
• lifecycle – group includes constructor()
• everything-else
• render

so the expected order of methods is:

foo          // instance-methods
classMethod  // instance-methods
constructor  // lifecycle
bar          // everything-else (static method)
render       // render

The test passes, but it should fail, because constructor() is placed before classMethod() in the source code, but it's expected to be after it.

The reason why the test passes is because classMethod() is being wrongly included in the everything-else group (it should be in instance-methods), and so the order of methods is:

foo          // instance-methods
constructor  // lifecycle
classMethod  // everything-else (wrong group!)
bar          // everything-else (static method)
render       // render

---

This PR fixes the bug affecting instanceMethod in checkPropsOrder(), and updates the corresponding unit tests (valid and invalid).

PS: I was the 1,000th forker of this repo :slightly_smiling_face:

[ljharb]

@michaelhogg an arrow function in a class property is resoundingly not an instance method - it's simply a data property that happens to have an arrow function (which, for the record, is a bad practice - never use arrow functions in class properties). Thus, a class field should not be sorted along with actual instance methods.

[michaelhogg]

@ljharb: The bug fix in this PR isn't regarding arrow functions in class properties.  This is the bug I've fixed:

class Hello extends React.Component {

    // instanceMethod == false
    classMethod() {}

}

checkPropsOrder() is wrongly classifying classMethod() with instanceMethod == false, and so it's being wrongly excluded from the instance-methods group :x:

[michaelhogg]

@ljharb: Your point about arrow functions in React component class properties is actually an interesting topic for discussion.

Currently, checkPropsOrder() does classify them as instance-methods (and my PR does not change this functionality):

class Hello extends React.Component {

    // instanceMethod == true
    bar = () => {}

}

TL;DR

There are basically two perspectives on this situation regarding arrow functions in class properties:

• @ljharb: I completely agree with you that they're not "instance methods" (since they're not defined on the class's prototype), and that they have definite disadvantages.  (I'm aware that you're a TC39 member with 4 successful ECMAScript proposals, so I have much respect for your point of view!)
• On the other hand, it's a very popular way (maybe the most popular way) of binding a function to a React component class instance, to create what many React developers might (loosely and inaccurately) refer to as an "instance method" (ie: a non-static function which can access the instance's context, in similar ways to a true "class method").

Regarding the sort-comp rule, I think React developers are more likely to expect such functions to be included in the instance-methods group, rather than the instance-variables group.

But to avoid the debate/uncertainty about whether they're instance-methods or instance-variables (and to give more granularity to the sort-comp rule), it might be better to use a different set of group names instead, such as:

• prototype-methods
• instance-functions
• instance-bound-functions
• instance-properties

1. prototype, performance, testability

As the AST nodes indicate here, bar() is a ClassProperty, not a MethodDefinition:

class Main {
    foo() {}        // MethodDefinition
    bar = () => {}  // ClassProperty
}

Babel stage-2 transpiles this to:

class Main {
    constructor() {
        this.bar = () => {};
    }
    foo() {}
}

In ES5, it becomes:

function Main() {
    this.bar = function () {};
}

Main.prototype.foo = function () {};

So bar() is not a "class method", because it's not defined on Main.prototype, and so it can't be called using super: :warning:

typeof Main.prototype.foo  // == "function"
typeof Main.prototype.bar  // == "undefined"

class Child extends Main {
    baz() {
        super.foo();  // OK
        super.bar();  // TypeError: (intermediate value).bar is not a function
    }
}

There are two other problems:

• There's a performance impact because the bar() function gets defined multiple times (once for each instance of Main) :warning:
• It harms testability :warning: @ljharb I've seen some of your comments on this subject: https://github.com/facebook/react/issues/9851#issuecomment-362026580, https://github.com/airbnb/enzyme/issues/365#issuecomment-217355830.

Here are some good articles on this issue:

• Arrow Functions in Class Properties Might Not Be As Great As We Think
• Of Classes and Arrow Functions (a cautionary tale)

2. Popularity in the React community

The usage of arrow functions in class properties is actually included in React's official documentation here and here.

It seems this technique is currently very popular in the React community.  Here's a Twitter poll from Oct 2017 in which it was voted the most popular technique for binding functions:

When doing a Google search for "react binding functions", some of the most popular results describe this technique as:

• having "multiple advantages"
• being the "best solution"
• "the recommended way to bind your callbacks"
• "a much cleaner way" with "notable benefits"
• "the best of both worlds"

I think it wouldn't be so popular if React developers were generally impacted by the disadvantages of using such functions.

For example, in my opinion, the inability to call them from a child class using super isn't much of a problem for React components, since React recommends composition over inheritance, and super is usually only used in the constructor() (when calling the React.Component parent constructor with super(props)).

It'd be good to raise awareness of the potential performance impact and testability problems, but in general, I think the popularity of this technique is likely to continue.

3. The meaning of the instance-methods group

The strict meaning of the word method is "a function defined on a class's prototype".

But given the popularity of using arrow functions in React component class properties, my guess is that many React developers would consider foo() and bar() to both be instance-methods, because they both have access to the class instance's context — they can both access the instance's properties (such as this.props and this.state) and component methods (such as this.setState() and this.forceUpdate()).  I guess this is sufficient for the needs of many React developers, and they wouldn't be too concerned that bar() isn't actually a "class method".

I'm not saying it's a good thing for any JS developer to be ignorant of important details of the JS language :stuck_out_tongue: I'm saying that when a React developer is reading this rule's documentation, in my opinion, they'll instinctively assume that arrow functions in React component class properties are more likely to be included in the instance-methods group, rather than the instance-variables group.

4. Some proposals for groups

@le0nik and @jozanza made a good point in https://github.com/yannickcr/eslint-plugin-react/issues/599#issuecomment-233215247 and https://github.com/yannickcr/eslint-plugin-react/issues/599#issuecomment-234097052:

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.

it would be really nice to have a syntax that disambiguates static, prototype, instance props/methods.

Maybe we could consider replacing the existing instance-methods and instance-variables groups with more granular groups like these? (to enable ESLint users to have full control over the ordering of all kinds of functions):

class Main extends React.Component {

    aaa() {}              // "prototype-methods"

    bbb = function () {}  // "instance-functions"

    ccc = () => {}        // "instance-bound-functions"

    ddd = 42;             // "instance-properties"

}

(This would be a breaking change, since the instance-methods and instance-variables groups have already been released in v7.6.0).

[ljharb]

@michaelhogg thanks for the in-depth response :-)

ok, so:

1. classMethod() {} should indeed be considered an "instance method". This is a good bug fix - however, it's possible this will be noisy enough that we might want to consider it semver-major just in case.
2. bar = () => {} is not an instance method (unrelated to any best practices discussion about arrows in class properties) and classifying it as such is a bug that we should fix asap. I think it should only be an "instance variable", and we should not differentiate by "bound methods" because foo = this.foo.bind(this) would also be included, so it makes things more complex.
3. The popularity of a bad pattern is a justification for linter rules and style guides to be stricter about it, to promote education. The React docs should be updated as well.
4. Renaming the existing groups to be both less correct, but potentially more clear to people who don't know how the language works, is an interesting (definitely semver-major) proposal.
5. Defaulting to include instance methods is a good change, but a semver-major one.

This PR should only include maybe item 1, and definitely item 2. The others should be filed as separate issues and discussed separately :-)

[TSMMark]

Hey guys how will this affect users of fat-arrow / instance-bound-functions in the future?

[dejoean]

There is a potential issue with including arrow functions in instance-variables with no separation, since the behavior of instance-variables in terms of class properties is equivalent to assignments of class properties in the constructor, and I believe should be placed directly after the constructor in the sort comp.

If we were to add arrow functions to this sorting group without the option to separate them, then we would essentially be forcing users to choose between the broken behavior of defining all class properties after componentWillUnmount, even though they are essentially an analogue for what is done in the constructor, or the broken behavior of defining all arrow function methods prior to the React lifecycle methods, which, while technically correct in terms of how arrow functions on the class are initialized and stored, would lead to cluttering of the component initialization code and a lack of separation between bound methods and class properties.

If we want to encourage users to separate their arrow function methods from other methods in order to inform developers that they are stored differently on instances, I would prefer it if we could create a separate group for arrow function methods (regardless of the name chosen) and place them either directly after componentWillUnmount or directly before render (since bound methods are most commonly used when methods need to be passed as props in React), and that we move instance-variables (or a new, separate group just for non-function class properties) directly below the constructor.

We could also note in the documentation that arrow function methods are created and stored separately on each instance of a class etc. if we want to educate developers about the difference.