6

I've recently seen this type of react pattern where the state is being set in a render by using this.state:

class ShowMe extends React.Component {

    constructor(props) {
        super(props);

        this.state = {
            showButton: false,
        };
    }

    render() {
        if (this.props.show) {
            this.state.showButton = true; //setting state in render!!
        }

        return (
            <div>
                <div> Show or hide button </div>
                {this.state.showButton && <Button content='Btn'/>}
            </div>
        )
    }
}

This seems like an anti-pattern. Can this cause bugs? It seems to work properly though.

I would just use a component lifecycle to set the state:

class ShowMe extends React.Component {

    constructor(props) {
        super(props);

        this.state = {
            showButton: false,
        };
    }

    componentWillReceiveProps(nextProps) {
        if(nextProps.show) {
            this.setState({
                showButton: true,         
            })
        }
     }

    render() {
        return (
            <div>
                <div> Show or hide button </div>
                {this.state.showButton && <Button content='Btn'/>}
            </div>
        )
    }
}

What is the recommended way?

user2456977
  • 3,830
  • 14
  • 48
  • 87
  • 1
    As the other answers mention, setting state inside `render` is an anti-pattern and could end up causing infinite loops. Additionally, setting state directly (eg. `this.state.foo = 'bar'`) instead of using `this.setState({foo: 'bar'})` can have unintended or unexpected consequences. Using the lifecycle method `componentWillReceiveProps` like you have done there is a much better pattern. – grammar Apr 18 '18 at 15:04

3 Answers3

4

render should always be pure without any side effects, so it's certainly a bad practice.

from the React docs :

The render() function should be pure, meaning that it does not modify component state, it returns the same result each time it’s invoked, and it does not directly interact with the browser. If you need to interact with the browser, perform your work in componentDidMount() or the other lifecycle methods instead. Keeping render() pure makes components easier to think about.

Take a look also here and here.

Hamza Fatmi
  • 1,235
  • 8
  • 10
4

It is an anti-pattern. If showButton state is not always equal to show props (which is the case in the example), I would use this:

class ShowMe extends React.Component {

    constructor(props) {
        super(props);

        this.state = {
            showButton: this.props.show,
        };
    }

    componentDidUpdate(prevProps, prevState) {
        prevProps.show !== this.props.show && this.setState({showButton: this.props.show})
    }

    render() {

        return (
            <div>
                <div> Show or hide button </div>
                {this.state.showButton && <Button content='Btn'/>}
            </div>
        )
    }
} 

Edit: As of React 16.3 one should use getDerivedStateFromProps in this case.

Note that componentWillReceiveProps will be deprecated.

From the docs: getDerivedStateFromProps is invoked after a component is instantiated as well as when it receives new props. It should return an object to update state, or null to indicate that the new props do not require any state updates.

https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops

Ali Ankarali
  • 2,761
  • 3
  • 18
  • 30
  • Is there a reason you prefer `componentDidUpdate` over `componentWillReceiveProps`? – user2456977 Apr 18 '18 at 15:32
  • 1
    I updated my answer they will deprecate componentWillReceiveProps and introduced getderivedstatefromprops for when props changes trigger state changes. Except that, componentWillReceiveProps would seem more natural over componentDidUpdate for it checks props while the later checks both props and state change. But I initially replied with componentDidUpdate because I thought you would first want your props to be updated and then update your state. – Ali Ankarali Apr 18 '18 at 15:45
1

It is incorrect setting state in render method. You can set state in lifecyles method. But other thing is that your component can receive same props many times, so your component will be set state many times, and renderd. To solve this problem you need to compare your new with your current props for example compare json objects:

componentWillReceiveProps(nextProps) {
    if(JSON.stringify(this.props) !== JSON.stringify(nextProps) && nextProps.show) {
        this.setState({
            showButton: true,         
        })
    }
 }

or use PureComponent. And that garentee you that your component will not rerendered constantly.

And it will be better if you do not rerender component if state.showButton currently seted to true.

componentWillReceiveProps(nextProps) {
    if(JSON.stringify(this.props) !== JSON.stringify(nextProps) && nextProps.show) {
       if(!this.state.showButton) {
         this.setState({
            showButton: true,         
        })
       }

    }
 }
Artem Mirchenko
  • 2,140
  • 1
  • 9
  • 21