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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Jul 31, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/util/Components.js
View file
Open in desktop
Expand Up @@ -670,7 +670,8 @@ function componentRule(rule, context) {
case 'AssignmentExpression':
case 'Property':
case 'ReturnStatement':
case 'ExportDefaultDeclaration': {
case 'ExportDefaultDeclaration':
case 'ArrowFunctionExpression': {
return true;
}
case 'SequenceExpression': {
Expand Down
66 changes: 47 additions & 19 deletions tests/lib/rules/no-this-in-sfc.js
View file
Open in desktop
Expand Up @@ -112,16 +112,6 @@ ruleTester.run('no-this-in-sfc', rule, {
};
}
}`
}, {
code: `
class Foo {
bar() {
() => () => {
this.something();
return null;
};
}
Comment on lines -116 to -123
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 •
edited

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 •
edited

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

}`
}, {
code: `
class Foo {
Expand All @@ -131,15 +121,6 @@ ruleTester.run('no-this-in-sfc', rule, {
};
}`,
parser: parsers.BABEL_ESLINT
}, {
code: `
class Foo {
bar = () => () => {
this.something();
return null;
};
}`,
parser: parsers.BABEL_ESLINT
}, {
code: `
export const Example = ({ prop }) => {
Expand All @@ -151,6 +132,21 @@ ruleTester.run('no-this-in-sfc', rule, {
};
};`,
parser: parsers.BABEL_ESLINT
}, {
code: `
export const prepareLogin = new ValidatedMethod({
name: "user.prepare",
validate: new SimpleSchema({
}).validator(),
run({ remember }) {
if (Meteor.isServer) {
const connectionId = this.connection.id; // react/no-this-in-sfc
return Methods.prepareLogin(connectionId, remember);
}
return null;
},
});
`
}],
invalid: [{
code: `
Expand Down Expand Up @@ -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 •
edited

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.

bar() {
return () => {
this.something();
return null;
}
}
}`,
errors: [{message: ERROR_MESSAGE}]
}, {
code: `
class Foo {
bar = () => () => {
this.something();
return null;
};
}`,
parser: parsers.BABEL_ESLINT,
errors: [{message: ERROR_MESSAGE}]
}, {
code: `
class Foo {
Expand All @@ -230,5 +247,16 @@ ruleTester.run('no-this-in-sfc', rule, {
}
}`,
errors: [{message: ERROR_MESSAGE}]
}, {
code: `
class Foo {
bar() {
() => () => {
this.something();
return null;
};
}
}`,
errors: [{message: ERROR_MESSAGE}]
}]
});