Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(interval): restart timer when timeout changes #140

Merged
merged 2 commits into from
May 10, 2019
Merged

fix(interval): restart timer when timeout changes #140

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63287b2
fix(interval): restart timer when timeout changes
jedwards1211 Apr 12, 2018
9ccc77d
Merge branch 'master' into patch-1
nkbt May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/interval/src/Component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ export class ReactInterval extends React.Component {
);
}

componentDidUpdate({enabled}) {
if (this.props.enabled !== enabled) {
componentDidUpdate({enabled, timeout}) {
if (this.props.enabled !== enabled || this.props.timeout !== timeout) {
Copy link
Owner

@nkbt nkbt Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with callback is if you change callback on the fly, then it will not restart the timer. Same issue as for timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah =( this is why we never use inline functions, or have custom sCU that totally ignore any functions and only care about values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. In any case I don't think any further changes are necessary here

if (this.props.enabled) {
this.start();
} else {
Expand Down