Turnipdabeets
Turnipdabeets

Reputation: 6015

Better way to clearTimeout in componentWillUnmount

I have a working Loading Component that cancels out when it has been loading for 8 seconds. This code works but it feels off to me and I'm wondering if there is a better way to do this.

Without setting this.mounted I get the error:

Warning: Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op. Please check the code for the Loading component.

This make me think that the timer is not getting canceled so it continues with this.seState. Why would that be if I set clearTimeout in componentWillUnmount? Is there a better way to handle this than using a global this.mounted?

class Loading extends Component {
  state = {
    error: false,
  };

  componentDidMount = () => {
    this.mounted = true;
    this.timer();
  };

  componentWillUnmount = () => {
    this.mounted = false;
    clearTimeout(this.timer);
  };

  timer = () =>
    setTimeout(() => {
      (this.mounted && this.setState({ error: true })) || null;
    }, 8000);

  render() {
    const { showHeader = false } = this.props;
    const { error } = this.state;
    return (
      <View style={backgroundStyle}>
        {showHeader && <HeaderShell />}
        {!error &&
          <View style={loadingHeight}>
            <PlatformSpinner size="large" />
          </View>}
        {error && <Error code="service" />}
      </View>
    );
  }
}

Loading.propTypes = {
  showHeader: PropTypes.bool,
};

Loading.defaultProps = {
  showHeader: false,
};

export default Loading;

Upvotes: 23

Views: 26147

Answers (3)

T.J. Crowder
T.J. Crowder

Reputation: 1075655

This make me think that the timer is not getting canceled

As Pointy said, it isn't. You're passing a function (this.timer) into clearTimeout. You need to pass the setTimeout return value (the handle of the timer), so you can use that handle to cancel it.

In such a simple componennt, I don't see the need for the timer function, it just adds complexity; I'd just set up the timer in CDM:

class Loading extends Component {
    state = {
        error: false,
    };

    componentDidMount = () => {                 // ***
        // Remember the timer handle            // ***
        this.timerHandle = setTimeout(() => {   // ***
            this.setState({ error: true });     // ***
            this.timerHandle = 0;               // ***
        }, 8000);                               // ***
    };                                          // ***
                                                // ***
    componentWillUnmount = () => {              // ***
        // Is our timer running?                // ***
        if (this.timerHandle) {                 // ***
            // Yes, clear it                    // ***
            clearTimeout(this.timerHandle);     // ***
            this.timerHandle = 0;               // ***
        }                                       // ***
    };                                          // ***

    render() {
        const { showHeader = false } = this.props;
        const { error } = this.state;
        return (
            <View style={backgroundStyle}>
                {showHeader && <HeaderShell />}
                {!error && (
                    <View style={loadingHeight}>
                        <PlatformSpinner size="large" />
                    </View>
                )}
                {error && <Error code="service" />}
            </View>
        );
    }
}

Loading.propTypes = {
    showHeader: PropTypes.bool,
};

Loading.defaultProps = {
    showHeader: false,
};

export default Loading;

But if there's more logic than shown, or just personal preference, yes, separate functions are good:

class Loading extends Component {
    state = {
        error: false,
    };

    componentDidMount = () => {
        this.setTimer();
    };

    componentWillUnmount = () => {
        this.clearTimer();
    };

    setTimer = () => {
        if (this.timerHandle) {
            // Exception?
            return;
        }
        // Remember the timer handle
        this.timerHandle = setTimeout(() => {
            this.setState({ error: true });
            this.timerHandle = 0;
        }, 8000);
    };

    clearTimer = () => {
        // Is our timer running?
        if (this.timerHandle) {
            // Yes, clear it
            clearTimeout(this.timerHandle);
            this.timerHandle = 0;
        }
    };

    render() {
        const { showHeader = false } = this.props;
        const { error } = this.state;
        return (
            <View style={backgroundStyle}>
                {showHeader && <HeaderShell />}
                {!error && (
                    <View style={loadingHeight}>
                        <PlatformSpinner size="large" />
                    </View>
                )}
                {error && <Error code="service" />}
            </View>
        );
    }
}

Loading.propTypes = {
    showHeader: PropTypes.bool,
};

Loading.defaultProps = {
    showHeader: false,
};

export default Loading;

About setting timerHandle to 0 when there's no active timer: It makes a convenient flag (see clearTimer above), and 0 is guaranteed not to be a valid timer handle by the timers specification. (For Node.js, you'd use null, since setTimeout returns an object, not a number as it does in browsers.)

Upvotes: 42

stackdave
stackdave

Reputation: 7094

React 16.3: this solution worked for me, the anothers solutios did not work in my case:

class Modal extends Component {

  constructor(props) {
    super(props);
    this.timeout = null;
    this.state = {
      clicked: false,
    };
  }

  handleClick = async (e, action) => {
    if (!this.state.clicked) { /
      this.setState( {clicked: true} , async () => {
        const res = await action();
        this.timeout = setTimeout(() => {
        if (this.mounted) this.setState( {clicked: false});
          this.timeout = null;
        }, 2000);

      });
    }
  };

  componentDidMount = () => {
    this.mounted = true;
  };


  componentWillUnmount = () =>{
    this.mounted = false;
    if (this.timeout) {
      clearTimeout(this.timeout)
    }
  };

Upvotes: 1

Dan Mason
Dan Mason

Reputation: 2337

You need to clear using the returned value from setTimeout (see below). However, doing clearTimeout in componentWillUnmount is a correct way to do it, I haven't seen anyone do it any differently.

  componentDidMount = () => {
    this.mounted = true;
    this.timeout = this.timer();
  };

  componentWillUnmount = () => {
    this.mounted = false;
    clearTimeout(this.timeout);
  };

  timer = () =>
    setTimeout(() => {
      (this.mounted && this.setState({ error: true })) || null;
    }, 8000);

Upvotes: 6

Related Questions