-
Notifications
You must be signed in to change notification settings - Fork 27.5k
feat($timeout): pass arguments in callback #10631
Conversation
Note that according to MDN, IE9 does not support the additional arguments. |
due to the way angular is handling |
@gdi2290: Good point ;) BTW, if it gets merged, it would be good to have the docs updatd as well (imo). |
5f941eb
to
b49404e
Compare
I added docs
|
deferred = (skipApply ? $$q : $q).defer(), | ||
promise = deferred.promise, | ||
timeoutId; | ||
|
||
timeoutId = $browser.defer(function() { | ||
try { | ||
deferred.resolve(fn()); | ||
deferred.resolve(fn.apply(null, args)); |
There was a problem hiding this comment.
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());
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
this and #10632 look good, but I would like to see a few more tests that make sure that ngMock also works as expected |
a73988e
to
d743f20
Compare
LGTM |
Feel free to merge |
I need to update this PR due to the API change of $timeout 5a60302 but I should be good to go soon |
ae4720e
to
7476a02
Compare
@petebacondarwin PR updated with rebase from master branch. Resolving merge conflicts are a pain I have to say. Since the |
4c4cfe1
to
1472151
Compare
setTimeout allows you to pass arguments into the callback function in the …3rd argument. Since that’s taken I added it to the …4th mdn.io/setTimeout#Syntax
1472151
to
76b7b48
Compare
Welcome to our world (rebase hell)! I think it is fine not to optimize the case where no handler function is passed for now. Merging |
Similar to how [`setTimeout`](mdn.io/setTimeout#Syntax) works, this commit allows users of `$timeout` to add additional parameters to the call, which will now be passed on to the callback function. Closes angular#10631
Similar to how [`setTimeout`](mdn.io/setTimeout#Syntax) works, this commit allows users of `$timeout` to add additional parameters to the call, which will now be passed on to the callback function. Closes angular#10631
PR has updated Docs and Tests
setTimeout
allows you to pass arguments into the callback function inthe …3rd argument. Since that’s taken I added it to the …4th
http://mdn.io/setTimeout#Syntax
here is an example of why you would need to do this
similar pull request #10632