react/prefer-stateless-function false positive

[jacobp100]

Input

class Test extends React.Component {
  constructor() {
    super();
    this.dispatchThingViaRedux = () => this.props.dispatch(thing());
  }

  render() {
    return <button onClick={this.dispatchThingViaRedux}>Click me!</button>;
  }
}

This gives a lint error to prefer a stateless function. However, in this case, it is not appropriate. While you could make a stateless function and just create a dispatch function every time, the value for the button onClick handler is changed between updates (i.e. newProps.onClick. !== props.onClick). Hence DOM updates have to occur if a stateless function was used.

[0x80]

Out of curiosity, why don't you write your component as a function and wrap it with connect to bind the thing function to dispatch? Seems much cleaner to me.

import {connect} from 'react-redux'
import {thing} from './actions'

function Test ({thing}) {
    return <button onClick={thing}>Click me!</button>;
  }

export default connect(null, {thing})(Test)

[ljharb]

If a component has instance properties, or uses this to refer to things that aren't props, then it has state, and shouldn't be an SFC. This is definitely a false positive.

@0x80, that is totally an option for refactoring, but the rule can't and shouldn't try to catch situations like that.

[jacobp100]

@0x80 neat! Didn’t know that was an option.

[0x80]

@jacobp100 You're welcome :)

[yannickcr]

If a component has instance properties, or uses this to refer to things that aren't props, then it has state, and shouldn't be an SFC. This is definitely a false positive.

I guess you're right. Does checking for this usage in the component (excluding this.props) could be enough to know if it should be an SFC or not ? (in addition to the other checks ofc)

[ljharb]

Things that should disqualify a component from being an SFC in this linter rule:

• the presence of any instance methods
• the presence of a non-useless constructor
• any reference to this besides this.props or const { props } = this
• any return null in render()

In other words, a class-based component should only be an SFC when:

• it has solely a render method (ignoring useless constructors)
• the only instance property referred to is this.props
• render() never returns null (or false or undefined or return; or return '';)

[jacobp100]

Can confirm @0x80's solution worked!

[vdh]

I've run into some false positives with classes that make use of onClick or onSubmit handlers to avoid recreating functions inside render. For example:

import React, { Component, PropTypes } from 'react';
import pureRender from 'pure-render-decorator';


@pureRender
export default class Example extends Component {
  static propTypes = {
    action: PropTypes.func.isRequired,
    id: PropTypes.number.isRequired,
  };

  handleClick = () => this.props.action(this.props.id);

  render() {
    return <button onClick={this.handleClick} />;
  }
}

While all the data is coming from props, I can't see any way it could be rewritten as a stateless function without breaking the jsx-no-bind rule.

[silvenon]

Yes, using callbacks should justify not using stateless components.

[yannickcr]

Following @ljharb advice's I rewrote the rule and it should now better target the components that should be written as SFC.

The new version will ask you to write a component as a SFC if:

• it has only displayName, propTypes, render and/or useless constructor (same detection as ESLint no-useless-constructor rule)
• the only instance property referred to is this.props and/or this.context (destructuring is handled)
• render() never returns anything than JSX (should cover all falsy values)

I also added much more tests.

Let me know I missed something.

[ljharb]

@yannickcr awesome! When React v15 comes out, you'll need to conditionally modify that last one, but for now this is 👍

[yannickcr]

@ljharb what is changing in v15 for the last one ?

[ljharb]

@yannickcr i thought I saw someone saying that SFCs could return falsy values/null in v15, but now I can't find the comment, nor a mention in the React blog post save https://facebook.github.io/react/blog/#rendering-null-now-uses-comment-nodes - so maybe nothing's changing ¯_(ツ)_/¯

[yannickcr]

@ljharb yep, I saw nothing in the blog post so I was wondering. But I added a shared setting for React version in 426b228 so it should not be very difficult to adapt for v15 if we need to.

[benoitgrelard]

@vdh I can see a way of writing it as a stateless component. I have actually used this myself.

Using your example, I would do the following by making use of a simple closure:

import React, { PropTypes } from 'react';
import pureRender from 'pure-render-decorator';


export default pureRender(Example);

function Example(props) {
  return <button onClick={_handleClick} />;

  function _handleClick() {
    props.action(props.id);
  }
}

Example.propTypes = {
  action: PropTypes.func.isRequired,
  id: PropTypes.number.isRequired
};

Also, thanks to hoisting, you can still export the pureRender'ed version up top.

So, to be honest, I'm not sure I agree with this thread that they are false positives.
Or am I missing something?

[vdh]

@artisologic That would still re-create _handleClick every render. To quote the description of the jsx-no-bind rule:

A bind call or arrow function in a JSX prop will create a brand new function on every single render. This is bad for performance, as it will result in the garbage collector being invoked way more than is necessary.

So even if the code doesn't trigger jsx-no-bind, the performance penalty from creating functions inside render still exists.

As a side note, I like to use the no-use-before-define rule because I feel hoisting adds unnecessary complication, and breaks the top-to-bottom flow of logic in a file.
Additionally, stateless components currently don't support pure render / shouldComponentUpdate (Before now I assumed they were already pure by nature of being functional, but it appears I was wrong about that).

[silvenon]

@vdh I'm pretty sure they are pure by definition, maybe that wasn't comunicated well by the React docs, otherwise we would all rather use classes.

In an ideal world, most of your components would be stateless functions because in the future we’ll also be able to make performance optimizations specific to these components by avoiding unnecessary checks and memory allocations. This is the recommended pattern, when possible.

[benoitgrelard]

Hi @vdh, I realised my mistake in my sleep and woke up this morning thinking... "The handler in the closure will still be recreated everytime!"

I have to say then, I find less and less use cases for using stateless components then apart from merely template components. Anything that would accept a callback is basically out of the question now.

Also, thanks for the details regarding the fact that stateless components can't support pure render, I had assumed they were given their name. I think it's correct that the doc is a bit misleading.

[Kerumen]

What about decorators ?

I have a class with the CSSModules decorator attached and it triggers react/prefer-stateless-function but I can't write it as a SFC because babel throws Leading decorators must be attached to a class declaration.

@CSSModules(styles)
class Button extends Component {
  render() {
    <button styleName="button"></button>
  }
}

[benoitgrelard]

@Kerumen decorators are just functions so your code is equivalent to:

class Button extends Component {
  render() {
    <button styleName="button"></button>
  }
}

export default CSSModules(styles)(Button);

I'm not sure what your HOC does but technically it should be possible for Button to be a SFC like so:

function Button(props) {
  return <button styleName="button"></button>;
}

export default CSSModules(styles)(Button);

[Kerumen]

Yep right! I always wrote the decorator so I forgot the good ol' syntax. Thanks :)

[silvenon]

Just double-checked about those stateless components and you guys are right, they aren't pure. My world just shattered 😢

[vdh]

@silvenon my world was also shattered, although there might be future optimisations to React that could enable automatic (or opt-in) pure rendering for stateless components.

@artisologic I've found that stateless components are best used for simple/small components that literally just map props (& context) to some JSX. For the non-trivial components I just stick with classes (and disable prefer-stateless-function per-file while I wait for the rule fix).

Also I notice that while react-css-modules is kind enough to return the original component, I couldn't spot anything in the ES7 spec that guarentees a return value from a decorator? They mutate their input, so I think they might need to be written like this:

@decoratorWithoutParam
@decoratorWithParam(param)
class Button extends Component {
  render() {
    return <button>{this.props.children}</button>;
  }
}

// bonus fat arrow + destructuring version
const Button = ({ children }) => (<button>{children}</button>);
decoratorWithoutParam(Button);
decoratorWithParam(param)(Button);

export default Button;

[vdh]

Hmm, on a second inspection it looks like there is some reassignment going on with some decorators, although it's sometimes problematic if multiple decorators try to wrap/reassign the same component. The decorator docs have examples that mutate (e.g. readonly), but also shows reassignment in the desugaring segment. So I guess until decorators get more stable in regards to reassignment vs mutation, "results may vary"?

[Robinnnnn]

While using Redux, I receive a false positive when I have a component that only uses render() but also uses connect + mapStateToProps + mapDispatchToProps.

class MyComponent extends React.Component {
  render() {
    const { prop1, prop2 } = this.props;
    return (
      <div>{prop1} {prop2}</div>
    )
  }
}

const mapStateToProps = state => ({
  prop1: state.prop1,
  prop2: state.prop2,
})

const mapDispatchToProps = dispatch => ({
  actions: bindActionCreators(actionCreators, dispatch)
})

export default connect(mapStateToProps, mapDispatchToProps)(MyComponent);

As far as I know, it is impossible to map state to a stateless function (duh). However, the linter continues to suggest to use a stateless component because I'm only calling render().

Let me know if I'm missing something obvious!

[ljharb]

@Robinnnnn It's mapStateToProps - the stateless function receives props, not state - the store's state is what is being mapped. You absolutely can (and should) convert MyComponent to an SFC.

[Robinnnnn]

@ljharb sorry, but do you mind showing me how?

[ljharb]

@Robinnnnn

function MyComponent({ prop1, prop2 }) {
  return (
    <div>{prop1} {prop2}</div>
  );
}
MyComponent.propTypes = {
  prop1: React.PropTypes.node,
  prop2: React.PropTypes.node,
};

const mapStateToProps = ({ prop1, prop2 }) => ({
  prop1,
  prop2,
});

const mapDispatchToProps = dispatch => ({
  actions: bindActionCreators(actionCreators, dispatch),
});

export default connect(mapStateToProps, mapDispatchToProps)(MyComponent);

[Robinnnnn]

Thank you!!