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

[jmdsg]

Hi, i have the following file which does not have a stateless functional component, it does not involve react at all but is showing me the error react/no-this-in-sfc when i call this.connection.

import { ValidatedMethod } from "meteor/mdg:validated-method";
import { SimpleSchema } from "meteor/aldeed:simple-schema";

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

[ljharb]

cc @alexzherdev any chance this is fixed already on master?

[alexzherdev]

No, it's not. This is not in a class, nor in a class method, so our checks are not triggering here. Perhaps we might add a check for a member in an object expression? But then what about this valid, albeit likely rare, use case 😕

const Namespace = {
  Component(props) {
    return <div />;
  }
};

<Namespace.Component />

[ljharb]

In this case, we’re determining it’s an SFC because it’s a function that destructured it’s first parameter, and sometimes returning null - but I’d expect that being a property of an object - especially if it’s an object passed to a constructor or a function - should downgrade its confidence so that it doesn’t report. Additionally, the function’s name is not PascalCased.

[alexzherdev]

I don't think we're doing any checks on destructured parameters - but returning JSX or null, sure.

Your point about function's name is interesting. I wonder why this has not been implemented before - I can't think of any drawbacks here. Although I'd probably just say Capitalized, not necessarily PascalCased, just to be safe. I feel like this would fix a lot of false positives.

[ElvenMonky]

Sorry, but I don't see why my comment was marked an off-topic.
Once again, most SFC are arrow functions, which can't have names. So if you'd like to take function name into consideration for "react/no-this-in-sfc" - do it with caution.

[ljharb]

Most SFCs aren’t necessarily arrow functions, because the Airbnb style guide forbids them - either way, the absence of a name surely won’t mean it’s not an SFC, but the presence of one might.

[AnabelSalomone]

I've seen a fix for an issue like this one was made (here: #1995 ), any chance we can have it in a release soon?

[ljharb]

v7.12.0 is released. Is this issue still a problem?

[paul-sachs]

@ljharb i have confirmed that this is still a problem as of v7.14.3 Going to checkout the code and see why exactly it's identifying my example as a functional component.

@connect(state => ({
  someReduxState: state.someReduxState
}))
export default class GroupWebUserWizard extends BaseComponent {
  renderFooter() {
    return <div>{this.formatMessage(...)}</div>
  }
}

[paul-sachs]

I realize my mistake, my example looks like this:

export default class {
  renderFooter() {
    return () => (
      <div>{this.value}</div>
    );
  }
}

Which is the thing that threw me.

[ljharb]

Your arrow function there is considered an SFC.

[paul-sachs]

I thought so as well, but apparently this passes just fine:

export default class {
  renderFooter = () => () => (
      <div>{this.value}</div>
    );
  }
}

But this doesn't:

export default class {
  renderFooter = () => {
    return () => (
      <div>{this.value}</div>
    );
    }
  }
}

The only difference is that one has a block that returns a function instead of the shorthand for returning a function.

[ljharb]

interesting, I’d expect both, or neither, to be detected as a component.

[isachivka]

export function wrapWithContext(context, contextTypes, children) {
  class ContextWrapper extends Component {
    static childContextTypes = contextTypes;

    constructor(props) {
      super(props);
      // eslint-disable-next-line react/no-this-in-sfc
      this.state = {};
    }

    getChildContext() {
      return context;
    }

    setProps = (props) => {
      // eslint-disable-next-line react/no-this-in-sfc
      this.setState({ ...props });
    };

    render() {
      return (
        // eslint-disable-next-line react/no-this-in-sfc
        cloneElement(children, this.state)
      );
    }
  }
  return createElement(ContextWrapper);
}

[ljharb]

No idea why setProps is an arrow function there (functions should never go in class fields) but certainly that's a clear example of a bug in the rule, thanks!