Eslint-plugin-react: Incorrect prop-types complaint on functions that destructure params and return jsx node

[sfletche]

See the comments in the getMessage function in the following jsx file.

import React, { PropTypes } from 'react';

function MyComponent({ name }) {
  const someParams = {
    userName: name,
    greeting: 'Hi there',
  };

  function getMessage({ userName, greeting }) {
    // THE FOLLOWING RETURN STATEMENT CAUSES LINTING COMPLAINTS
    // 9:25 error  'userName' is missing in props validation  react/prop-types
    // 9:35 error  'greeting' is missing in props validation  react/prop-types
    return <span>`${greeting} ${userName}`</span>;

    // WHILE THIS ONE DOES NOT
    // return `${greeting} ${userName}`;
  }

  return (
    <div>
      { getMessage(someParams) }
    </div>
  );
}

MyComponent.propTypes = {
  name: PropTypes.string.isRequired,
};

export default MyComponent;

relevant packages include

"eslint": "3.14.1",
"eslint-config-airbnb": "13.0.0",
"eslint-plugin-import": "2.2.0",
"eslint-plugin-jsx-a11y": "2.2.3",
"eslint-plugin-react": "6.9.0",

[ljharb]

There's not really a good way to detect SFCs without treating all jsx-returning-functions as components.

getMessage is a function that returns jsx, so it should instead be an SFC (from an idiomatic React standpoint).

[sfletche]

@ljharb: I can see how it would be difficult (if not impossible) to accurately detect an SFC 100% of the time...

What about a partial solution that detects potential SFCs through the presence of the propTypes property?
(in the above example, MyComponent has a propTypes property, while getMessage does not)

While this (and other potential mitigating strategies) would not prevent all false negatives, they would decrease the number of false negatives.

[ljharb]

Then the rules requiring that all used props have propTypes wouldn't be able to catch components where propTypes has been forgotten.

[sfletche]

Ah. I see the problem...

Has there been any consideration to decomposing the rule?
Maybe one rule that detects when propTypes are not declared on a component, and another rule that detects props that are not among the declared propTypes.

Or an option that would trigger missing props validations only when propTypes is declared on a component?

[nfantone]

Same problem over here.

PropTypes are clearly declared but errors are popping out.

[ljharb]

The idiomatic approach here in React is to always make a component rather than a jsx-returning function. Doing so just happens to avoid issues with this plugin's component detection.

@sfletche all the rules have to work in concert. Certainly we could add a global setting that would treat functions that have no React statics attached as non-SFCs, but that doesn't seem like a satisfying solution.

Is there a reason that both of you can't change your function to an SFC?

[nfantone]

@ljharb In my case, it _is_ a component that happens to call a bunch of JSX generating functions. I'd expect this to be a common approach? Sure, I _could_ turn those functions into a new component - but I sincerely see no point from a usability POV.

[ljharb]

It would allow you to add propTypes to each of those subcomponents, test them separately, and allow React to do more optimization on the render tree.

At any rate that's orthogonal to the issue here, for which we don't have a good solution.

[nfantone]

Point(s) taken. I still find it to be a bit of a stretch, really. I'm sure there are plenty of things like these in the wild:

const getName = (user) => (<li>user.name</li>);

I may be wrong here, but I believe it's not realistic to expect every single JSX-returning-function to be turned into a whole new component.

[ljharb]

That's our policy in Airbnb's codebase and it tends to work out just fine, ftr.

At any rate, one possible component detection modification could be that if a jsx-returning-function that has no React statics (propTypes, defaultProps, contextTypes, childContextTypes, etc) is both invoked as a normal function, and not used in a jsx element context, in the same file, we could treat it as "not a component". That seems like it'd cover at least a few of these use cases without reducing the utility towards actual components?

[lencioni]

@nfantone FWIW, turning this into a component:

const getName = (user) => (<li>user.name</li>);

could be as simple as adding curlies in the signature:

const getName = ({ user }) => (<li>user.name</li>);

and updating the callsites of course.

[lancempoe]

@ljharb If the policy at AirBnB is to only have components return jsx, how would you handle the case below where we are returning one component or the other depending on a check.

import React, { PropTypes } from 'react';

function MyComponent({ name }) {
  const someParams = {
    userName: name,
  };

  function getMessage({ userName }) {
    // THE FOLLOWING RETURN STATEMENT CAUSES LINTING COMPLAINTS
    // 9:25 error  'userName' is missing in props validation  react/prop-types

     if (userName === 'something') {
       return <MyReactComponent/>;
     } else {
       return <MyOtherReactComponent/>;
     }
  }

  return (
    <div>
      { getMessage(someParams) }
    </div>
  );
}

MyComponent.propTypes = {
  name: PropTypes.string.isRequired,
};

export default MyComponent;

[ljharb]

@lancempoe i'd either inline that logic directly inside render, or i'd make it a separate component.

[lancempoe]

I have always felt (and heard) that having logic in the render is an antipattern. Is AirBnB advocating for logic to be within a render?

[ljharb]

Logic in a render (or a logic function invoked in a render) is totally fine! As long as render is purely based on props and state (pure in the no-side-effects sense) then it's great.

[bogdan-zaharia-hs]

It seems that it would also be a performance issue with turning the functions into SFC.

Also, maybe a way to detect a SFC could be to check that the name is in TitleCase instead of camelCase.

[ljharb]

That naming convention also identifies, by convention, any function used as a constructor, so there's no way that would work.

In general, don't worry about performance at all until after everything is correct, working, and tested. SFCs are more appropriate than renderFoo methods; use them until your app has speed issues.