feat($interval) add a service wrapping setInterval #4047
Conversation
|
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
|
@juliemr - looking through the code there seems to be a load of It was originally committed by Misko and then refactored by Vojta. This stuff is only used by the $cookie service to poll, as I understand it, the browser's cookies to keep them synch with what the cookie store holds. If we are moving over to the $interval service then I would imagine it would be a good idea to refactor all this code too and remove all this |
| if (promise && promise.$$intervalId in intervals) { | ||
| intervals[promise.$$intervalId].reject('canceled'); | ||
| delete intervals[promise.$$intervalId]; | ||
| return $browser.cancelRepeatTask(promise.$$intervalId); |
There was a problem hiding this comment.
I appreciate that it doesn't really matter, but the order of the delete and the call to cancelRepeatTask is different here compared to when the interval resolves naturally above. To avoid confusion I would probably swap the earlier pair of statements.
|
Nice @juliemr - Taking into account my comments above - looks good to me. |
| * @name ng.$browser#addRepeatTask | ||
| * @methodOf ng.$browser | ||
| * @param {function()} fn A function that should be executed repeatedly. | ||
| * @param {number=} [delay=1000] milliseconds between executions. |
There was a problem hiding this comment.
does it really make sense to set the default delay? how meaning full is it?
There was a problem hiding this comment.
Hm, not really meaningful - you should be aware of how often something will execute when you set the function. Made it non-optional.
|
Don't be discouraged by many comments I left. I really like most of the PR. thanks for the great work! |
|
What's the status on this? Looks like all of your comments have been addressed, @IgorMinar. |
ghost
assigned 
|
It's hard to see in the outdated diffs, so reiterating this comment: My only concern is that now there's a lot of duplication between the reimplementation and the real $interval service (and also duplication in their tests). Any ideas, or is this repetition acceptable since one is in ngMock? |
|
@juliemr the mock could "decorate" the original service, instead of defining it again. (the duplicate tests should be preserved imho). |
|
@juliemr can you make sure all the comments are addressed and all the commits squashed into reasonable units (and following the msg convention)? Otherwise LGTM. |
|
@vojtajina can you please just make the changes and get this in? |
The $interval service simplifies creating and testing recurring tasks. This service does not increment $browser's outstanding request count, which means that scenario tests and Protractor tests will not timeout when a site uses a polling function registered by $interval. Provides a workaround for angular#2402. For unit tests, repeated tasks can be controlled using ngMock$interval's tick(), tickNext(), and tickAll() functions.
|
Squashed! Having the mock decorate instead of redefining doesn't save any duplication (the way the service is currently implemented), since every method of the mock needs to be slightly different than the method of the original. Let me know if you think there's too much duplication now, but I'm fine with it as is. |
| iteration = 0, | ||
| skipApply = (isDefined(invokeApply) && !invokeApply); | ||
|
|
||
| promise.then(null, null, fn); |
There was a problem hiding this comment.
Shouldn't we return this promise instead ?
If the user processes the returned value (the iteration number in this case), this promise returned by $interval won't get it:
$interval(function(couter) {
return 'processed value';
}).then(null, null, function(counter) {
counter !== 'processed value'
});There was a problem hiding this comment.
I thought about it a bit more and I think the current behavior is correct, because:
- the
fncallback should not be in the flow of the promise - if there's an exception in the callback it should still notify - if the creator really wants to transform the counter, it is still possible:
var p = $interval(function() {...});
return p.then(null, null, function(counter) {
return 'transformed counter';
});
|
I added some line notes on 2b5ce84 regarding apparently missing |
|
Pete, I'm just curious... Are you still adding new features to 1.2.0? I thought that it was feature complete. There are a lot of pending PR with interesting features that seem to be postponed after the 1.2.0 launch, why is this one different? |
|
Ok! Thanks for the answer Brian |
The $interval service simplifies creating and testing recurring tasks.
This service does not increment $browser's outstanding request count,
which means that scenario tests and Protractor tests will not timeout
when a site uses a polling function registered by $interval. Provides
a workaround for #2402.
For unit tests, repeated tasks can be controlled using ngMock$interval's
tick(), tickNext(), and tickAll() functions.