Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($interval): call fn even if invokeApply is false #5903

Conversation

minznerjosh
Copy link

Becasue calling the interval function relies on a promise notify
handler, the interval function would not get called without
invokeApply being true. With this update, if invokeApply is false, the
function is called normally.

Closes #5902

Becasue calling the interval function relies on a promise notify
handler, the interval function would not get called without a
invokeApply being true. With this update, if invokeApply is false, the
function is called normally.

Closes angular#5902
@minznerjosh
Copy link
Author

Also, I'm guessing that, for the same reason above, this won't work either:

var logTenTimes = $interval(function() {
    console.log('foo');
}, 100, 10, false);

logTenTimes.then(function() {
    console.log('DONE!'); // Won't get called because of lack of $digest().
});

So... Should a $digest() happen on the last iteration to resolve the promise even if invokeApply is false? I would imagine so, right? I can update my PR if I hear that this is the desired behavior!

@ghost ghost assigned IgorMinar Jan 22, 2014
@IgorMinar
Copy link
Contributor

Interesting! @juliemr and I totally missed this.

if $apply is disabled then the whole promise-based api is going to behave in all sorts of different weird ways.

it might be better to just not return a promise if $apply is disabled.

since this is a breaking change, I'm going to schedule it for 1.3

@minznerjosh
Copy link
Author

@IgorMinar Thanks so much for taking a look! :-) Would you like me to update the PR to not return the promise if invokeApply is false (and update the docs as well,) or would you like me to wait for others to weigh in?

Although, if $interval doesn't return a promise, it'll need to return something else that can be passed to $interval.cancel() to stop the interval.

Either way, this makes the API pretty janky. Because, depending on what is decided that $interval() should return if invokeApply is false, you either end up with:

  1. A promise that doesn't call your notifyHandler (and resolveHandler as of now)
  2. Some other object/value to use with $interval.cancel()

Off the top of my head, I can't think of any place in Angular where the return value of a service method changes drastically based on a fairly basic parameter like invokeApply. Granted, setting invokeApply to false is probably an edge case, but it just doesn't feel quite right.

My humble suggestion is to just remove the invokeApply parameter from $interval(). If you find yourself wanting to create an interval, but want to opt-out of the $digest cycle, just use $window.setInterval. It seems like the only advantages to using $interval with invokeApply false would be the promise-based API (which fundamentally won't work because you need a $digest,) a nicer API for canceling intervals and easy testing with $interval.flush.

Personally, I find $interval.flush to be the most important/useful advantage. Maybe the ngMockWindow object could be updated to include a method for flushing normal setIntervals?

Just thinking out loud here. Any thoughts?

@minznerjosh
Copy link
Author

@IgorMinar Never-mind. It seems angular.mock.createMockWindow() was deprecated in 1.2?

Maybe it'd just be best to have $interval(angular.noop, 1000, 0, false) return a numerical ID that can be passed to $interval.cancel(). I suppose that wouldn't we so bad as it would be mimicking the behavior of the underlying window.setInterval and window.clearInterval.

What do you think?

@Narretz
Copy link
Contributor

Narretz commented Mar 13, 2014

@IgorMinar 1.3 is here! Can we move this forward?

@rodyhaddad
Copy link
Contributor

Related #7999

@caitp
Copy link
Contributor

caitp commented Jun 27, 2014

As demonstrated in #5902 this issue appears to have already been fixed, #7999 fixes a different issue, but doesn't re-break this one as far as I can tell

@caitp caitp closed this Jun 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$interval: Function is Not Called if invokeApply is False
5 participants