Apply no-this-in-sfc rule only for modules with react import

[sergei-startsev]

Fixed #1967 issue.

[alexzherdev]

Note: this assumes an SFC will always have some JSX, but it's not always the case. E.g.

const ConditionalComponent = ({ children }) => {
  if (someCondition) {
    return children;
  }
  return null;
}

[sergei-startsev]

That's true, otherwise you cannot narrow the scope of the rule. Based on what I can see in the issue we wouldn't like to affect modules that aren't not related to React, wouldn't we?

[ljharb]

The issue in #1967 is because it's inside a class field, and also because it's being returned from a function itself. The solution is to fix component detection so it has a low confidence when either of these things is true.

[sergei-startsev]

@ljharb I'm a bit confused, should the following code be handled by the rule and considered as invalid?

function Foo() {
   if (this.foo) {
      something();
   }
   return null;
}

For now it triggers lint errors, however the function isn't related to React and I suppose it shouldn't be considered as a component.

[ljharb]

@sergei-startsev yes, that could be a react component. however, #1967 isn't related to that snippet.

[sergei-startsev]

Thanks, I'll take a look

[alundiak]

I'm on [email protected] and I also have that described issue. No React/JSX code AT ALL.

[sergei-startsev]

@ljharb I've created a separate PR #1989 to fix the particular issue with component detection for arrow functions inside a class field.

cc @alexzherdev

[sergei-startsev]

@ljharb Could we back to the example above:

function Foo() {
   if (this.foo) {
      something();
   }
   return null;
}

Foo can be a stateless React component, but it also can used as a constructor const f = new Foo(). It's not related to React, however there's no possibility to check it with static analysis. I'm thinking about adding an option to handle only modules with React import to prevent false positives. @ljharb Thoughts?

[ljharb]

In this case, it’s a function that uses no jsx, doesn’t take any props argument, and references this - that seems like it should have a low enough confidence that it’s not treated as a component.

[sergei-startsev]

OK, let's say it takes some arguments:

function Foo({something}) {
   if (this.bar) {
      return something;
   }
   return null;
}

Can it be a component? There's no guarantee that it cannot be used as a component. It also can be used as a constructor.
I suppose it can be beneficial to provide an option to ignore modules if there's no confidence that they have a React component. E.g. I wouldn't like to run the rule for the entire code base if I have thousands modules that aren't related to React (and there's no strict rule regarding placement React components in the file system -- feature based convention is used instead).

[ljharb]

If it’s accessing this, and not using it for props/state/context, it’s probably not a component, even for this rule.

[sergei-startsev]

OK, here's a more realistic example:

const Placeholder = ({children}) => {
    if (this.ready) {
        return children;
    }
    return null;
};

Is it not a component? I just replaced something by children and bar by ready.

I think if we could provide an option to just ignore such cases (it's turned off by default), it should allow to avoid possible confusion and make the rule more flexible.

[ljharb]

That seems like yes, it is a component.

I think it’s fine if you have a function that uses this, that has no clear indicators that it’s not a component, in a codebase that’s using React, to force the use of disable directives. The majority case will be that it’s trivial to apply React linting rules only to a React part of the codebase.

[sergei-startsev]

The majority case will be that it’s trivial to apply React linting rules only to a React part of the codebase.

I don't think that's always true, but I'll close PR for now, let's see if the community raise it again.

[ljharb]

I still think there’s an opportunity here to improve our detection heuristics, though. Just because I’m comfortable with false positives doesn’t mean i don’t also want to minimize them.