renatorib / react-powerplug

:electric_plug: Renderless Containers
https://renatorib.github.io/react-powerplug
MIT License
2.68k stars 101 forks source link

Fix setting multiple interval error #176

Closed beizhedenglong closed 5 years ago

TrySound commented 6 years ago

This is discussable.

@renatorib We need to decide do we want to ignore start with existing one or do we want to restart interval after calling start?

beizhedenglong commented 6 years ago

Every time delay changes, it will clear old interval and restart with new interval automatically. Every time we call start, it just leave the old interval running and replace intervalId with new one. Is it necessary to clear old one and replace with new one have the same functionality ?

renatorib commented 6 years ago

I do not know if I understood the idea here. What is the problem?

beizhedenglong commented 6 years ago

I do not know if I understood the idea here. What is the problem?

@renatorib if we call start multiple times. We can only stop the last interval. And the previous intervals still running, we can’t stop them

renatorib commented 6 years ago

Oh, I got it. So, I think clearInterval before set a new is better, what you think?

  _setIntervalIfNecessary = delay => {
    if (Number.isFinite(delay)) {
      this._clearIntervalIfNecessary()
      this.intervalId = setInterval(
        () => this.setState(s => ({ times: s.times + 1 })),
        delay
      )
    }
  }

This way, start() will work as restart

beizhedenglong commented 6 years ago

It's ok. But ...

_setIntervalIfNecessary = delay => {
  if (Number.isFinite(delay)) {
    this._clearIntervalIfNecessary()
    //  If we already have an interval `a`, this will create a new interval has the same functionality  that `a` has.
    // Because We always remove old interval and create a new one in componentDidUpdate phrase, when delay changes.
    this.intervalId = setInterval(
      () => this.setState(s => ({ times: s.times + 1 })),
      delay
    )
  }
}

componentDidUpdate(prevProps) {
  // already removed old interval here.
  if (prevProps.delay !== this.props.delay) {
    this.stop()
    this.start()
  }
}

I think just ignoring start and leaving existing interval running is better.

renatorib commented 6 years ago

I think I prefer keep start working as 'restart'. Powerplug was born to be practical, to be uncomplicated. I think it's better.

beizhedenglong commented 6 years ago

it's fine to restart interval when calling start

renatorib commented 5 years ago

Nice! I requested this change in code review so we can merge, okay? Thanks!

renatorib commented 5 years ago

Thanks @beizhedenglong