feat($debounce): add new service that "debounces" calls to a function #6749
Conversation
This is a useful service in general but, in particular, it also is needed to support the `ng-model-options` directive, that allows users to "debounce" the ui event triggers that update the ng-model value.
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
| expect(counter).toBe(2); | ||
| })); | ||
|
|
||
| it('should return a promise which will be resolved with the result of the call to the wrapped function', |
There was a problem hiding this comment.
some funky indentation in this file in a few places
- Add option to skip $apply - Only run $apply when the wrapped function is called, and not when we are cancelling timeouts. -
|
Wow! That was fast. |
| * @param {boolean} [immediate=false] If true then `fn` is invoked on the first call to | ||
| * the debounced function. Otherwise the first call will not | ||
| * happen until after the `delay` time has expired | ||
| * @param {boolean=} [invokeApply=true] If set to `false` skips model dirty checking, otherwise |
There was a problem hiding this comment.
invokeApply=false doesn't make sense if you are exposing a promise based api. we already have a problem with $interval and there is a hack to workaround this in $timeout I think. Aren't you making the same mistake?
There was a problem hiding this comment.
Is that because our $q framework always wraps resolves in $apply blocks? That didn't seem to be the case in my unit testing but then unit tests are not real life with respect to digests.
There was a problem hiding this comment.
I think you need to call $browser.defer.flush() (to emulate Q's nextTick())... the $timeout and $interval mocks cover that, you probably want to add a mock for the $debounce service
Basically this https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L1647-L1649 although in your own tests I think $timeout.flush() should be enough
|
|
otherwise this looks good |
We are not going to expose promises from this service for now since it is not a common use case and it is easier to add them in later than to remove them if they cause problems. Moreover it makes the code considerable simpler. The code will implicitly call $apply if it calls the wrapped function asynchronously, as it is inside the $timeout handler. The code will not explicitly call $apply if it is calling the wrapped function synchronously (i.e. if `immediate` is `true`) - we will leave it up to the calling environment to control that. In most cases the caller will already be in an apply block.
|
Fixed up those changes requested by @IgorMinar. The code is much much simpler now, easier to follow and will run much faster. |
| var context = this, args = arguments; | ||
|
|
||
| // This is the function that will be called when the delay period is complete | ||
| var delayComplete = function() { |
There was a problem hiding this comment.
does this function meed to be recreated every time the debounced function is called? (it may not matter a whole lot)
There was a problem hiding this comment.
I guess I can move it up a level to the $debounce function and pass in context and args.
Remove unneeded parameters and move delayComplete outside of debouncedFn.
|
Fixed up one more time. |
|
@petebacondarwin, please consider:
To see a working example of those two points, you can take a look at my debounce service/directive implementation at: https://github.com/shahata/angular-debounce which sadly I wrote last night while not knowing that those features are going to make it into angular so soon :) |
|
@shahata - nice service you have written! I especially like the comprehensive tests. For our purposes (i.e. for ng-model-options) the two features you suggest are not necessary right now. |
|
Sure, I totally agree with this approach and I'd be happy send those PRs. Regarding the return value, I thought that you might want to start with it since you chose to support the 'immediate' param (I guess in order to be compatible with the existing debounce implementation in underscore, since I don't think it is an important use case). Regarding the cancel support, you might want to read my comment in #2129. I think it is pretty important... |
This is a useful service in general but, in particular, it also is needed
to support the
ng-model-optionsdirective, that allows users to "debounce"the ui event triggers that update the ng-model value.
See #2129