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

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

Closed
PatrickJS 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/ng/timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Copy Markdown
Member

@gkalpak gkalpak Jan 4, 2015

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
Copy Markdown
Contributor

@shahata shahata Jan 4, 2015

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
Copy Markdown
Contributor Author

@PatrickJS PatrickJS Jan 4, 2015

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
Copy Markdown
Contributor

@shahata shahata Jan 4, 2015

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
Original file line number Diff line number Diff line change
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