New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@gdi2290
Member

gdi2290 commented Jan 4, 2015

PR has updated Docs and Tests
setTimeout allows you to pass arguments into the callback function in
the …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

import {$http} from 'angular.js/modules';
import {setTimeout, Promise} from 'facade/async';
var cache = {};

export class AwesomeModule {
  static getSwag(url:string):Promise {
    return new Promise(function(resolve, reject) {
      var complete = function(data) {
        cache[url] = data;
        resolve(data);
      };

      if (url in cache) {
        setTimeout(complete, 0, cache[url]);
      } else {
        $http.get(url).then(complete, reject);
      } // end url in cache
    });
  } // end getSwag
} // end AwesomeModule

similar pull request #10632

@googlebot googlebot added the cla: yes label Jan 4, 2015

@gkalpak

This comment has been minimized.

Member

gkalpak commented Jan 4, 2015

Note that according to MDN, IE9 does not support the additional arguments.

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Jan 4, 2015

due to the way angular is handling setTimeout intervally this code works in IE9 even if setTimeout doesn't support it natively

@gkalpak

This comment has been minimized.

Member

gkalpak commented Jan 4, 2015

@gdi2290: Good point ;) BTW, if it gets merged, it would be good to have the docs updatd as well (imo).

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Jan 4, 2015

I added docs

* @param {...*=} Pass additional parameters to the executed function.
deferred = (skipApply ? $$q : $q).defer(),
promise = deferred.promise,
timeoutId;
timeoutId = $browser.defer(function() {
try {
deferred.resolve(fn());
deferred.resolve(fn.apply(null, args));

This comment has been minimized.

@gkalpak

gkalpak Jan 4, 2015

Member

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());

This comment has been minimized.

@shahata

shahata Jan 4, 2015

Contributor

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.

This comment has been minimized.

@gdi2290

gdi2290 Jan 4, 2015

Member

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));

This comment has been minimized.

@shahata

shahata Jan 4, 2015

Contributor

👍

@lgalfaso

This comment has been minimized.

Member

lgalfaso commented Jan 4, 2015

this and #10632 look good, but I would like to see a few more tests that make sure that ngMock also works as expected

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Jan 21, 2015

LGTM

@petebacondarwin petebacondarwin added this to the 1.4.0-beta.2 / 1.3.11 milestone Jan 21, 2015

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Jan 21, 2015

Feel free to merge

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Jan 22, 2015

I need to update this PR due to the API change of $timeout 5a60302 but I should be good to go soon

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Jan 22, 2015

@petebacondarwin PR updated with rebase from master branch. Resolving merge conflicts are a pain I have to say.

Since the $timeout overload doesn't affect this feature too much I didn't account for it in the code (no callback so no point providing noop it arguments). I could check if they're using method overload and use $browser.defer(noop, delay) rather than having it run all the logic needed for the original api. The other PR (5a60302) didn't do this optimization so I can assume it doesn't matter

feat($timeout): pass arguments in callback
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

@petebacondarwin petebacondarwin removed this from the 1.4.0-beta.5 / 1.3.14 milestone Feb 24, 2015

@petebacondarwin petebacondarwin self-assigned this Mar 3, 2015

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Mar 9, 2015

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

@gdi2290 gdi2290 deleted the gdi2290:timeout-args branch Mar 9, 2015

hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015

feat($timeout): pass additional arguments to the callback
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

netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

feat($timeout): pass additional arguments to the callback
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment