prop-types false positive on internal render method with a destructure parameter

[alangpierce]

In this code, I think the _renderValue method is being treated as a stateless functional component, and the prop-types rule is complaining about the value parameter:

const MyComponent = React.createClass({
  propTypes: {
    values: React.PropTypes.arrayOf(React.PropTypes.number.isRequired).isRequired,
  },

  _renderValue({value}) {  // Error: 'value' is missing in props validation (react/prop-types)
    return <span>{value}</span>;
  },

  render() {
    return (
      <div>
        {this.props.values.map((value) => this._renderValue({value}))}
      </div>
    );
  },
});

Note that this only seems to happen when there are destructured params and only when using React.createClass, not ES2015 classes. It seems like a reasonable fix (if I'm interpreting the problem correctly) would be to say that a function isn't an SFC if it's defined using method syntax.

[ljharb]

It is; and it should be an SFC instead.

You're right that skipping method syntax in component detection should help here; but renderFoo methods are an antipattern anyways.

[alangpierce]

renderFoo methods are an antipattern anyways

Yep, makes sense. This isn't new code; I'm fixing pre-existing lint issues in a big codebase and found this in two places, so in the near term I'll probably leave the code as-is and disable the rule with an inline comment.

I do think there's sometimes value in using methods as a low-overhead way to split up a render function without having to plumb down the props, state, and callbacks, particularly as an alternative to IIFEs or variables that would be defined at the start of render. But yeah, probably I should be biasing more toward SFCs than I do now, and ideally I'd have few enough props that SFCs don't feel like much overhead.

Anyway, would you be open to calling this a bug and accepting a change to skip method syntax when detecting SFCs?

[ljharb]

That does seem reasonable, yes.

[veob]

renderFoo methods are an antipattern anyways

@ljharb, why? Maybe you can point to some articles on the topic? Thanks.

[ljharb]

I don't have an article, but because you're expanding the public API on a component, with something that's conceptually important enough to separate yet somehow not important enough to you to have its own propTypes safety?

[otakustay]

In my case we have higher order functions returning Element for component's props, such as:

const getBodyCellRender = record => ({dataIndex, render}) => {
   return <td> ... </td>;
};

We can make it a "component factory" like:

const createBodyCellComponent = record => {
  const BodyCell = ({dataIndex, render}) => <td> ... </td>;
  BodyCell.propTypes = { ... };
  return BodyCell;
};

But this is really strange for me.

[ljharb]

That's what it is; a function that returns jsx is a component, and a function returning a function that returns jsx is an HOC/component factory.

[otakustay]

I don't think this statement is always right, for example:

const renderItem = ({name, count}) => <li>{name} ({count})</li>;

return (
  <ul>
    list.map(renderItem)
  </li>
);

Yes we can make it list.map(ListItem) but is it really nice and pretty?

The official redux realworld demo also makes a simple function/method returning react elements.

[ljharb]

In that case, your mapped items lack a key, so they’ll trigger a React error. What happens if you add a key?

[ljharb]

Our component detection is much improved since this was last updated.

If you're having issues with the latest version, please file a new issue.