Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

jzabala
Copy link
Contributor

@jzabala jzabala commented Jul 11, 2020

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.

Comment on lines -116 to -123
code: `
class Foo {
bar() {
() => () => {
this.something();
return null;
};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jzabala jzabala Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@jzabala jzabala Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
  );
}

@@ -217,6 +213,27 @@ ruleTester.run('no-this-in-sfc', rule, {
return <div onClick={onClick}>{this.props.foo}</div>;
}`,
errors: [{message: ERROR_MESSAGE}, {message: ERROR_MESSAGE}]
}, {
code: `
class Foo {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jzabala jzabala Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@ljharb ljharb force-pushed the fix/component-detection-inconsistency branch from 0ecc48e to bcfdbd5 Compare July 31, 2020 06:06
@ljharb ljharb changed the title [Fix]: add arrow function to list of allowed position for component [Fix] no-this-in-sfc/component detection: add arrow function to list of allowed position for component Jul 31, 2020
@ljharb ljharb merged commit bcfdbd5 into jsx-eslint:master Jul 31, 2020
@jzabala jzabala deleted the fix/component-detection-inconsistency branch July 31, 2020 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Non sfc function shows "react/no-this-in-sfc"
2 participants