[Fix] no-this-in-sfc/component detection: add arrow function to list of allowed position for component

[jzabala]

As reported in issue #2010 comment #2010 (comment) there is an inconsistency in the detection of components returned from functions with and without return statements.

Since arrow functions can do a direct return without return statement, this PR adds ArrowFunctionExpression to the list of allowed position for component.

Also adds a test for #2010 to show it is fixed.

Closes #2010.

[ljharb]

hmm, these aren't components, and the this here refers to the class instance. Why are these invalid?

[jzabala]

@ljharb
In the current test file there is the following example:

https://github.com/yannickcr/eslint-plugin-react/blob/4c8d8cc0a5dca39554cf97b807822b60782f320f/tests/lib/rules/no-this-in-sfc.js#L222

{
    code: `
    class Foo {
      bar() {
        function Bar(){
          return () => {
            this.something();
            return null;
          }
        }
      }
    }`,
    errors: [{message: ERROR_MESSAGE}]
  }

You can also try this one by removing the name of the function:

{
    code: `
    class Foo {
      bar() {
        return function () {
          return () => {
            this.something();
            return null;
          }
        }
      }
    }`,
    errors: [{message: ERROR_MESSAGE}]
  }

The purpose of this PR is to make the behavior consistent. If the two examples using function keyword are invalid why is the example using arrow function valid? In the two examples I showed the arrow functions are also using the this coming from the outer function and the outer functions are not React components since they don't return null or JSX.

[ljharb]

I think that any linting warning for this rule that's produced on "not a React component" is a bug. A class that doesn't extend React.Component (or have the appropriate jsdoc) should be ignored (altho if it contains a react component, that should be considered)

[jzabala]

I think that any linting warning for this rule that's produced on "not a React component" is a bug. A class that doesn't extend React.Component (or have the appropriate jsdoc) should be ignored (altho if it contains a react component, that should be considered)

Yeah, agree but the error is not on the class Foo but on the returned arrow function.

I suppose that in other to support HOC, functions that return jsx or null and, are returned by other function (using the return keyword) are considered components. For example:

function connect(Component) {
  return () => <Component>{this.props.one}</Component> // <-- considered a component
}

const connect = (Component) => {
  return () => <Component>{this.props.one}</Component> // <-- considered a component
}

function connect(Component) {
  return function() { // <-- considered a component
    return <Component>{this.props.one}</Component>
  }
}

const connect = (Component) => {
  return () => { // <-- considered a component
    return null;
  }
}

What this PR is trying to fix is that their counter part arrow functions (without return keyword) are not considered components:

const connect = (Component) => (
  () => <Component>{this.props.one}</Component>  // <-- not considered a component
)

const connect = (Component) => (
  function() {  // <-- not considered a component
    return <Component>{this.props.one}</Component>
  }
)

const connect = (Component) => (
  () => { // <-- not considered a component
    return null;
  }
)

[ljharb]

hmm, ok - but an arrow function that returns an arrow function that returns null doesn't seem likely to be a component to me.

[jzabala]

hmm, ok - but an arrow function that returns an arrow function that returns null doesn't seem likely to be a component to me.

Totally agree but, supporting HOC and functions returning null considered components makes to correctly identify components in this situation incredible hard. Maybe we could enforce in this situation the need for returning JSX and not just null.

Following your example about arrow function that returns arrow function that returns null, this is a component right now:

const connect = () => {
  return () => {
    return () => { // <-- a component
      return null;
    }
  };
}

But, the inconsistent behavior, this is not:

const connect = () => {
  return () => (
    () => { // <-- not a component
      return null;
    }
  );
}

[ljharb]

this isn't a react component, so i wouldn't expect anything in it to be warned on

[jzabala]

@ljharb
Similar to current test:

https://github.com/yannickcr/eslint-plugin-react/blob/4c8d8cc0a5dca39554cf97b807822b60782f320f/tests/lib/rules/no-this-in-sfc.js#L136

Foo is a class that defines the function bar and the function bar returns an arrow function that use this.

The only difference between this test and the mentioned one is that bar is not defined using an arrow function.

The purpose of this PR is to make the behavior consistent.