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($interval): pass arguments in callback #10632

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gdi2290
Member

gdi2290 commented Jan 4, 2015

PR has updated Docs and Tests
setInterval allows you to pass arguments into the callback function
in
the …3rd argument. Since that’s taken I added it to the …5th
http://mdn.io/setInterval#Syntax

similar pull request #10631

@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 setInterval intervally this code works in IE9 even if setInterval doesn't support it natively. I can optimise the code a bit more by checking if there are arguments before wrapping a function around it.

promise.then(null, null, function() {
  fn.apply(null, args);
});
promise.then(null, null, args.length === 0 ? fn : function() {
  fn.apply(null, args);
});
@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 and optimise the code a bit to check args once

* @param {...*=} Pass additional parameters to the executed function.
expect(task2).toHaveBeenCalledWith('Task2');
$window.flush(1000);
expect(task3).toHaveBeenCalledWith('I', 'am', 'a', 'Task3', 'spy');
}));

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 21, 2015

Member

This sequence of expectations seems not quite right. Since we are using $interval then each of the spies should have been called once per flush.

How about something more like:

$window.flush(1000);
expect(task1).toHaveBeenCalledWith('Task1');
expect(task2).toHaveBeenCalledWith('Task2');
expect(task3).toHaveBeenCalledWith('I', 'am', 'a', 'Task3', 'spy');
task1.reset();
task2.reset();
task3.reset();
$window.flush(1000);
expect(task1).toHaveBeenCalledWith('Task1');
expect(task2).toHaveBeenCalledWith('Task2');
expect(task3).toHaveBeenCalledWith('I', 'am', 'a', 'Task3', 'spy');

This comment has been minimized.

@gdi2290

gdi2290 Jan 22, 2015

Member

I updated the tests (and rebase master) with an $interval count of 2 otherwise the second part of this won't work since the .reset() only resets the state of the spy and not $interval.

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

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Jan 22, 2015

I have a question about the mocks. https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L456 I'm assuming I need to update the mocks with the same logic but it seems a bit crazy that I can add a feature to $interval without this showing up as an error. Then again I didn't run any of the e2e tests since my computer is throwing Java errors (gg Java) if it did account for it I assumed Travis would catch it

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Jan 22, 2015

@gdi2290 - the tests of the $interval service should not be relying on the mock version of $interval; so we ought to be able to check that the $interval service is correct.
But I agree that keeping mocks in sync with their real life counterparts. One approach, that perhaps you might like to investigate, is to see if we can run the $interval unit tests against the mock version of $interval.

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Jan 22, 2015

I pushed up changes to mock to reflect the $interval in order to get this merged. There are a few ways this can be solved

  • Refactoring to expose private methods setInterval and clearInterval so we can use a decorator similar to how ngMocks/$timeout#L1713 is handled in Angular 1.x or Angular 2 with facade/async.es6#L39
  • Hack Jasmine/tests to repeat the tests for mock/real
  • Update the build system to run tests for mocks/real (not sure if mocks are included into docs tests but again I have Java errors)

either one that's chosen I think a new PR would make sense for that change while this one should be good

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Mar 9, 2015

Actually setInterval and clearInterval are already exposed because $interval gets them from $window, which is a service that we can adapt. So perhaps instead of decorating $interval what we need to do is decorate or replace $window with new versions of these methods?

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Mar 9, 2015

In the meantime, this PR looks fine from the point of view of this feature, so let's land it too.

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

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

feat($interval): pass additional arguments to the callback
Similar to how [`setInterval`](http://mdn.io/setInterval#Syntax) works, this
commit allows users of `$interval` to add additional parameters to the call,
which will now be passed on to the callback function.

Closes angular#10632

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

feat($interval): pass additional arguments to the callback
Similar to how [`setInterval`](http://mdn.io/setInterval#Syntax) works, this
commit allows users of `$interval` to add additional parameters to the call,
which will now be passed on to the callback function.

Closes angular#10632
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment