When updating a stateful ponent in React is it considered a bad practice when a ponent uses the current state to update the new state.

For example if I have a class that stores whether a filter is open or not in it's state, is one of these options for updating the state more desirable than the other in terms of performance?

Option 1:

class Container extends Component {
    state = {
        show: false
    }

    show = () => this.setState({ show: true })

    hide = () => this.setState({ show: false })

    render() {
        <ExternalComponent
            show={this.show}
            hide={this.hide}
        />
    }
}

Option 2:

class Container extends Component {
    state = {
        show: false
    }

    toggleVisibility = () => this.setState({ show: !this.state.show })

    render() {
        <ExternalComponent
            toggleVisibility={this.toggleVisibility}
        />
    }
}

Option 3:

class Container extends Component {
    state = {
        show: false
    }

    setShow = (newVal) => this.setState({ show: newVal })

    render() {
        <ExternalComponent
            setShow={this.setShow}
        />
    }
}

When updating a stateful ponent in React is it considered a bad practice when a ponent uses the current state to update the new state.

For example if I have a class that stores whether a filter is open or not in it's state, is one of these options for updating the state more desirable than the other in terms of performance?

Option 1:

class Container extends Component {
    state = {
        show: false
    }

    show = () => this.setState({ show: true })

    hide = () => this.setState({ show: false })

    render() {
        <ExternalComponent
            show={this.show}
            hide={this.hide}
        />
    }
}

Option 2:

class Container extends Component {
    state = {
        show: false
    }

    toggleVisibility = () => this.setState({ show: !this.state.show })

    render() {
        <ExternalComponent
            toggleVisibility={this.toggleVisibility}
        />
    }
}

Option 3:

class Container extends Component {
    state = {
        show: false
    }

    setShow = (newVal) => this.setState({ show: newVal })

    render() {
        <ExternalComponent
            setShow={this.setShow}
        />
    }
}

• javascript
• reactjs
• setstate

• 4 I don't see why that would be considered bad practice other than that state changes are async and mergeable. Which is probably a reasonable concern--you might not get what you expect. Personally I'd prefer option #3. – Dave Newton Commented Feb 27, 2017 at 20:27
• It is indeed bad practice and there is a reasonable concern for the reason you mentioned, and it's outlined in the docs as well. – Christian Jensen Commented Nov 12, 2019 at 20:42

2 Answers 2

There is nothing wrong with a ponent accessing its own state. Write-only state wouldn't be terribly useful! However, you should be very careful when exposing ponent state or state-altering methods to other ponents. Component state is internal, and should only been touched from outside via a well-considered interface, to prevent your ponents from being an entangled mess.

In fact, there is an example that is similar to your Example #2 in the React documentation:

class Toggle extends React.Component {
  constructor(props) {
    super(props);
    this.state = {isToggleOn: true};

    // This binding is necessary to make `this` work in the callback
    this.handleClick = this.handleClick.bind(this);
  }

  handleClick() {
    this.setState(prevState => ({
      isToggleOn: !prevState.isToggleOn
    }));
  }

  render() {
    return (
      <button onClick={this.handleClick}>
        {this.state.isToggleOn ? 'ON' : 'OFF'}
      </button>
    );
  }
}

ReactDOM.render(
  <Toggle />,
  document.getElementById('root')
);

Note a difference from your example, however. The toggle method needs to be bound in the constructor, to ensure that this means what you're expecting it to mean.

If the wrapping ponent is the one keeping track of the visibility of the child ExternalComponent, then rather than passing the toggle method into the child ponent, I would expect the wrapper to render a hide/show affordance of some sort, and then either pass the current visibility into the child ponent as a prop or selectively render it (note that selective rendering will cause the entire child ponent to be remounted when it's enabled again, which may be expensive; you may be better off hiding it than tearing it down and recreating it). That makes the division of concerns clear: the wrapper knows about visibility, and the child ponent doesn't need to know how or why that decision was made, nor does it need to touch the wrapper's internal state.

There is nothing wrong with using the current state's value to determine the new state value.

Option 2 has less code, which kind of appeals to me. However, sometimes I might have to use Option 1 when using a third-party ponent (e.g. Semantic UI React modal) and it has show and hide handlers that we have to define.

Option 3 is also fine; I would use it for other applications other than this show/hide one (in fact, that one is used pretty much all the time, especially when you have controlled input ponents).