Rule Proposal: functions returning JSX should be refactored into React components

[keithamus]

Hey @yannickcr thanks for being awesome and making this great plugin, super valuable 😎.

I have noticed, that while working with a team of React devs - some devs will write functions that return JSX, but aren't components. This usually makes things difficult to refactor later on, as we need to split out all of these "jsx fragment functions" into real React components.

For example, we often see things like this slip into our codebase:

export default class ArticleList extends React.Component {

  renderImage(src, alt, caption) {
    return (
      <figure>
      <img src={src} />
      { caption ? <figcaption>{caption}</figcaption> : null }
      </figure>
    );
  }

  renderArticle(articles) {
    return articles.map((article) => (
      <a href={item.url}>{this.renderImage(item.src, item.alt, item.caption)}</a>
    ));
  }

  render() {
    return (
      <div class="article-list">
        {this.renderItems(this.props.articles)}
      </div>
    );
  }  
}

The problem with the above code, other than from a purity standpoint, is that it becomes a bit of a rats nest at any level of complexity, plus it is difficult to refactor when you need to. Ideally this code would be written as such:

function ArticleImage({ src, alt, caption }) {
  return (
    <figure>
    <img src={src} />
    { caption ? <figcaption>{caption}</figcaption> : null }
    </figure>
  );
}

function ArticleItem({ url, src, alt, caption }) {
  return (
    <a href={url}>
      <ArticleImage src={src} alt={alt} caption={caption}/>
    </a>
  );
}

export default function ArticleList({ articles }) {
  return (
    <div class="article-list">
      {articles.map((article) => <ArticleItem {...article} />)}
    </div>
  );
}

The above code is much easier to reason about - each function that returns JSX has a well defined function signature (that of a react component) and each piece can be easily pulled out into its own component if the time comes.

Proposal

All of the above is just to demonstrate the proposal for the following rule:

react/no-jsx-outside-component: If a function returns JSX, it should be a React component (class or function).

The rule would check all functions in a file:

• if the function returned JSX
• or if the function returned React.createElement calls
• or if the function returned map/reduce calls which themselves return JSX
• the function is not a React.Component#render method
• or the function does not follow the React stateless function signature (fn(props = {}))

Then the rule would warn that the function should be a React (stateless or class) component.

---

Let me know what your thoughts are on this. If you're happy with the above proposal I'm happy to put the work into a PR for this.

[yannickcr]

Seems to be an interesting rule.

It should be feasible, we already have a isReturningJSX utility function that can already do "jsx fragment functions" detection (but it doesn't cover all cases, like map/reduce). I think it could be used (and improved) for this.

I'm definitely interested in this rule. If you can come out with a good solution I'll be happy to merge it :)

[eballeste]

@keithamus wondering if you do anything different almost 2 years later or is this still the best approach for these type of scenarios?

[keithamus]

@eballeste I haven't used react in a while but if I recall, we ultimately resovled to only ever use functional stateless compoments for everything except for components needing the low level life cycle hooks which would naturally get much closer scruitiny. If I was maintaining any react codebases anymore that's the direction I'd certainly move in.

[dirkroorda]

Two problems:

1. Nowadays react components can also return strings and arrays.
2. What about functions that return react components? Like

const addProp = Component => props => <Component prop={'a'} {...props} />

This is not a component but a component wrapper.
I think in general, when you do a lot of functional manipulation with components, you will see quite a bunch of functions that have the signature of a component but are not meant as a component.

[ljharb]

@dirkroorda A component wrapper is itself an SFC component.

#1510 is for tracking support of React 16 fragments.

[dirkroorda]

@ljharb I thought that a component is a function with the signature ({ props }) => R or a class with a render function of that signature, where R is any piece of JSX.

The wrapper addProp above does not have that signature, so it is not a component.
On the other hand, now I think of it, its result is not JSX, but a function that delivers JSX, so it is not a good example.

[ljharb]

@dirkroorda the wrapper addProp above is a function that returns an SFC - an SFC factory.

[danny-andrews-snap]

The problem with refactoring JSX-returning functions into components is that it can break your tests if you are shallow rendering.

For example, in the first case, I could write test case, I might have written a test that asserted that images are rendered for each article. But when I refactored to components, that test would fail, since the ArticleItem and ArticleImage components would be stubbed out. Maybe this is just a shortcoming of shallow rendering?

[ljharb]

I’d argue that’s not breaking your tests, it’s improving them - by forcing you to split your tests up so that the new component has its own tests.

In other words, shallow rendering means each component primarily has tests that test what it renders, and they delegate testing responsibility for those things to their own tests.

[danny-andrews-snap]

Hmmm...I see your point in where you refactor out reusable components, but in the case where you just need "helper components" which will only ever be used by one component, they are analogous to private functions and, as such, shouldn't be tested directly.

[ljharb]

For that, enzyme has .dive()

[danny-andrews-snap]

Now your test is aware of the implementation details of your component. 🙁 Decide that helper component isn't needed, or split it into two, and your tests break again.

BTW, enzyme is great, and I love using it. ;)

[gunjarinagaraju]

Will u explain what are all the rules to be followed for writing jsx,higherorder function,rendermethod

[xsrvmy]

One potential concern with such a rule: the inline help function for .map inside render returns JSX without being used as a component. Are there any scenarios where similar logic needs to be implemented manually?

[Fuco1]

@xsrvmy An argument to map is an anonymous function, so this can distinguish it.

The moment it is assigned to either a variable or made into a function bla(), this can be detected and it should be required to become a component.

@dirkroorda

Nowadays react components can also return strings and arrays.

This is not a problem because the rule is: IF (return jsx) THEN (make component). If it returns a string, whatever.

[psyrendust]

@keithamus @yannickcr This has been open for about 7 years now. Just wondering there is any sort of timeline for this?

[ljharb]

@psyrendust #1580 has some outstanding comments going back 6 years, so no, there's no timeline until someone wants to pick it back up.

If you're interested, please comment a link to a rebased branch in that PR, and i'll pull in the changes.