fix(interval): restart timer when timeout changes #140
Conversation
| componentDidUpdate({enabled}) { | ||
| if (this.props.enabled !== enabled) { | ||
| componentDidUpdate({enabled, timeout}) { | ||
| if (this.props.enabled !== enabled || this.props.timeout !== timeout) { |
There was a problem hiding this comment.
Ah yeah, I see. Never really changed timeout on the fly
I would probably consider having this check in a separate function and reused in sCU. callback is also needed.
There was a problem hiding this comment.
Oh you're right, what am I thinking. We could accomplish that by removing the top-level if statement altogether, because sCU ensures that one of those props will have updated if componentDidUpdate gets called.
There was a problem hiding this comment.
Actually this PR is fine as it is because the callback prop is wrapped by the class property that calls it and sets the next timeout, so even if the callback changes, when the timer fires it will call the latest callback.
I don't understand why not just use an actual setInterval though.
There was a problem hiding this comment.
Because if task takes more time then interval tasks will start to overlap and queue. SetTimeout will not have this issue and will execute only when it actually capable
There was a problem hiding this comment.
The problem with callback is if you change callback on the fly, then it will not restart the timer. Same issue as for timeout.
There was a problem hiding this comment.
@nkbt I would say if the task takes that long it's probably a major problem with the user's code.
I think restarting the timer when the callback changes is probably a bad idea because if the user uses an inline function for the callback the timer will reset every time it gets a new inline function instance.
There was a problem hiding this comment.
Yeah =( this is why we never use inline functions, or have custom sCU that totally ignore any functions and only care about values
There was a problem hiding this comment.
Right. In any case I don't think any further changes are necessary here
fix #139