Eslint-plugin-react: 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]

Please, take into consideration, that SFC doesn't have to (or even shouldn't) have name at all.

Some might even restrict SFC to be default exported arrow function only.

[ljharb]

Every React component absolutely should have a name, for debugging purposes. No component (SFC or not) has to.

[ElvenMonky]

Debugging is not an excuse to pollute your code by dublicating identifiers. Module name can be primary identifier for the component. If one desires displayName, it can be attached explicitely using "displayName" property and in separate debugging module, rather than relying on React to implicitely infer displayName from class/function name, and mixing debug and production code in same modules. Same stands for prop-types.

[ljharb]

Who said anything about duplicating an identifier? displayName is the override mechanism - the primary method is a function’s name.

[ljharb]

Either way, this is off topic. What makes an SFC - or not one - is fact, not up for debate.

[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: https://github.com/yannickcr/eslint-plugin-react/pull/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!