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

feat($timeout): pass arguments in callback #10631

Closed
wants to merge 1 commit into from
Closed

feat($timeout): pass arguments in callback #10631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/ng/timeout.js
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@ function $TimeoutProvider() {
* @param {number=} [delay=0] Delay in milliseconds.
* @param {boolean=} [invokeApply=true] If set to `false` skips model dirty checking, otherwise
* will invoke `fn` within the {@link ng.$rootScope.Scope#$apply $apply} block.
* @param {...*=} Pass additional parameters to the executed function.
* @returns {Promise} Promise that will be resolved when the timeout is reached. The value this
* promise will be resolved with is the return value of the `fn` function.
*
Expand All @@ -43,14 +44,15 @@ function $TimeoutProvider() {
fn = noop;
}

var skipApply = (isDefined(invokeApply) && !invokeApply),
var args = sliceArgs(arguments, 3),
skipApply = (isDefined(invokeApply) && !invokeApply),
deferred = (skipApply ? $$q : $q).defer(),
promise = deferred.promise,
timeoutId;

timeoutId = $browser.defer(function() {
try {
deferred.resolve(fn());
deferred.resolve(fn.apply(null, args));
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if making the same optimisation as in $interval here would make any tangible difference. E.g.:

var hasArgs = arguments.length > 3;
...
deferred.resolve(hasArgs ? fn.apply(null, args) : fn());

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go the other way and make this change a simple one liner:

deferred.resolve(fn.apply(null, sliceArgs(arguments, 3)));

there's no need for this hasArgs variable and no need to test the arguments.length on your own, sliceArgs will take care of it just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have reference to arguments in that scope so the best I can do is sliceArgs regardless of argument length

//...
var args = sliceArgs(arguments, 3)
//...
  //...
  deferred.resolve(fn.apply(null, args));

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

} catch (e) {
deferred.reject(e);
$exceptionHandler(e);
Expand Down
67 changes: 57 additions & 10 deletions test/ng/timeoutSpec.js
View file
Open in desktop
Expand Up @@ -22,16 +22,16 @@ describe('$timeout', function() {
it('should call $apply after each callback is executed', inject(function($timeout, $rootScope) {
var applySpy = spyOn($rootScope, '$apply').andCallThrough();

$timeout(function() {});
$timeout(noop);
expect(applySpy).not.toHaveBeenCalled();

$timeout.flush();
expect(applySpy).toHaveBeenCalledOnce();

applySpy.reset();

$timeout(function() {});
$timeout(function() {});
$timeout(noop);
$timeout(noop);
$timeout.flush();
expect(applySpy.callCount).toBe(2);
}));
Expand All @@ -40,7 +40,7 @@ describe('$timeout', function() {
it('should NOT call $apply if skipApply is set to true', inject(function($timeout, $rootScope) {
var applySpy = spyOn($rootScope, '$apply').andCallThrough();

$timeout(function() {}, 12, false);
$timeout(noop, 12, false);
expect(applySpy).not.toHaveBeenCalled();

$timeout.flush();
Expand Down Expand Up @@ -89,8 +89,8 @@ describe('$timeout', function() {
// $browser.defer.cancel is only called on cancel if the deferred object is still referenced
var cancelSpy = spyOn($browser.defer, 'cancel').andCallThrough();

var promise1 = $timeout(function() {}, 0, false);
var promise2 = $timeout(function() {}, 100, false);
var promise1 = $timeout(noop, 0, false);
var promise2 = $timeout(noop, 100, false);
expect(cancelSpy).not.toHaveBeenCalled();

$timeout.flush(0);
Expand All @@ -104,7 +104,6 @@ describe('$timeout', function() {
expect(cancelSpy).toHaveBeenCalled();
}));


it('should allow the `fn` parameter to be optional', inject(function($timeout, log) {

$timeout().then(function(value) { log('promise success: ' + value); }, log.fn('promise error'));
Expand All @@ -123,6 +122,35 @@ describe('$timeout', function() {
expect(log).toEqual(['promise success: undefined']);
}));

it('should pass the timeout arguments in the timeout callback',
inject(function($timeout, $browser, log) {
var task1 = jasmine.createSpy('Nappa'),
task2 = jasmine.createSpy('Vegeta');

$timeout(task1, 9000, true, 'What does', 'the timeout', 'say about', 'its delay level');
expect($browser.deferredFns.length).toBe(1);

$timeout(task2, 9001, false, 'It\'s', 'over', 9000);
expect($browser.deferredFns.length).toBe(2);

$timeout(9000, false, 'What!', 9000).then(function(value) { log('There\'s no way that can be right! ' + value); }, log.fn('It can\'t!'));
expect($browser.deferredFns.length).toBe(3);
expect(log).toEqual([]);

$timeout.flush(0);
expect(task1).not.toHaveBeenCalled();

$timeout.flush(9000);
expect(task1).toHaveBeenCalledWith('What does', 'the timeout', 'say about', 'its delay level');

$timeout.flush(1);
expect(task2).toHaveBeenCalledWith('It\'s', 'over', 9000);

$timeout.flush(9000);
expect(log).toEqual(['There\'s no way that can be right! undefined']);

}));


describe('exception handling', function() {

Expand All @@ -133,7 +161,7 @@ describe('$timeout', function() {

it('should delegate exception to the $exceptionHandler service', inject(
function($timeout, $exceptionHandler) {
$timeout(function() {throw "Test Error";});
$timeout(function() { throw "Test Error"; });
expect($exceptionHandler.errors).toEqual([]);

$timeout.flush();
Expand All @@ -145,7 +173,7 @@ describe('$timeout', function() {
function($timeout, $rootScope) {
var applySpy = spyOn($rootScope, '$apply').andCallThrough();

$timeout(function() {throw "Test Error";});
$timeout(function() { throw "Test Error"; });
expect(applySpy).not.toHaveBeenCalled();

$timeout.flush();
Expand All @@ -164,6 +192,25 @@ describe('$timeout', function() {
}));


it('should pass the timeout arguments in the timeout callback even if an exception is thrown',
inject(function($timeout, log) {
var promise1 = $timeout(function(arg) { throw arg; }, 9000, true, 'Some Arguments');
var promise2 = $timeout(function(arg1, args2) { throw arg1 + ' ' + args2; }, 9001, false, 'Are Meant', 'To Be Thrown');

promise1.then(log.fn('success'), function(reason) { log('error: ' + reason); });
promise2.then(log.fn('success'), function(reason) { log('error: ' + reason); });

$timeout.flush(0);
expect(log).toEqual('');

$timeout.flush(9000);
expect(log).toEqual('error: Some Arguments');

$timeout.flush(1);
expect(log).toEqual('error: Some Arguments; error: Are Meant To Be Thrown');
}));


it('should forget references to relevant deferred even when exception is thrown',
inject(function($timeout, $browser) {
// $browser.defer.cancel is only called on cancel if the deferred object is still referenced
Expand Down Expand Up @@ -242,7 +289,7 @@ describe('$timeout', function() {
// $browser.defer.cancel is only called on cancel if the deferred object is still referenced
var cancelSpy = spyOn($browser.defer, 'cancel').andCallThrough();

var promise = $timeout(function() {}, 0, false);
var promise = $timeout(noop, 0, false);

expect(cancelSpy).not.toHaveBeenCalled();
$timeout.cancel(promise);
Expand Down