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

fix(ngModelOptions): introduce $cancelUpdate to cancel pending updates #7014

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
42 changes: 23 additions & 19 deletions src/ng/directive/input.js
Original file line number Diff line number Diff line change
Expand Up @@ -1577,7 +1577,8 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$

var ngModelGet = $parse($attr.ngModel),
ngModelSet = ngModelGet.assign,
pendingDebounce = null;
pendingDebounce = null,
ctrl = this;

if (!ngModelSet) {
throw minErr('ngModel')('nonassign', "Expression '{0}' is non-assignable. Element: {1}",
Expand Down Expand Up @@ -1693,19 +1694,26 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$

/**
* @ngdoc method
* @name ngModel.NgModelController#$cancelDebounce
* @name ngModel.NgModelController#$cancelUpdate
*
* @description
* Cancel a pending debounced update.
* Cancel an update and reset the input element's value to prevent an update to the `$viewValue`,
* which may be caused by a pending debounced event or because the input is waiting for a some
* future event.
*
* This method should be called before directly update a debounced model from the scope in
* order to prevent unintended future changes of the model value because of a delayed event.
* If you have an input that uses `ng-model-options` to set up debounced events or events such
* as blur you can have a situation where there is a period when the value of the input element
* is out of synch with the ngModel's `$viewValue`. You can run into difficulties if you try to
* update the ngModel's `$modelValue` programmatically before these debounced/future events have
* completed, because Angular's dirty checking mechanism is not able to tell whether the model
* has actually changed or not. This method should be called before directly updating a model
* from the scope in case you have an input with `ng-model-options` that do not include immediate
* update of the default trigger. This is important in order to make sure that this input field
* will be updated with the new value and any pending operation will be canceled.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this description would benefit from initially layout out the problem that this method solves. For instance:

If you have an input that uses ng-model-options to set up debounced events or events such as blur you can have a situation where there is a period when the value of the input element is out of synch with the ngModel's $viewValue. You can run into difficulties if you try to update the ngModel's '$modelValue` programmatically before these debounced/future events have completed, because Angular's dirty checking mechanism is not able to tell whether the model has actually changed or not.

Then you can go on with how to use this method, as you have described already.

this.$cancelDebounce = function() {
if ( pendingDebounce ) {
$timeout.cancel(pendingDebounce);
pendingDebounce = null;
}
this.$cancelUpdate = function() {
$timeout.cancel(pendingDebounce);
this.$render();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctrl.$render();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both will work. When do you prefer ctrl to this in such cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using ctrl instead of this within the methods everywhere for consistency. But we could keep this for whenever we are defining a new member, such as

this.$cancelUpdate = function() {...};

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll add this to this PR as a separate commit.

};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it when functions are this simple! It just makes the world seem right.


// update the view value
Expand Down Expand Up @@ -1764,25 +1772,21 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* @param {string} trigger Event that triggered the update.
*/
this.$setViewValue = function(value, trigger) {
var that = this;
var debounceDelay = this.$options && (isObject(this.$options.debounce)
? (this.$options.debounce[trigger] || this.$options.debounce['default'] || 0)
: this.$options.debounce) || 0;

that.$cancelDebounce();
if ( debounceDelay ) {
$timeout.cancel(pendingDebounce);
if (debounceDelay) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot on the parenthesis styling

pendingDebounce = $timeout(function() {
pendingDebounce = null;
that.$$realSetViewValue(value);
ctrl.$$realSetViewValue(value);
}, debounceDelay);
} else {
that.$$realSetViewValue(value);
this.$$realSetViewValue(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be ctrl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It is in the context of this.$setViewValue...

BTW, I was wondering - what do you think about using ctrl everywhere? It will be a bit less clear, but it would minify better. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone calls $setViewValue as a free function then this would fail, or am I wrong?

I think using ctrl everywhere is a good idea for general safety and consistency, although I suspect that a good minifier might swap out this anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uglify doesn't swap this and I believe minifiers try not to do that since it is risky (by this you could in theory actually be referring to the context in which the function was invoked and not the context in which it was defined, so swapping this automatically could result in breaking your code in a very hard to debug fashion :) )

}
};

// model -> value
var ctrl = this;

$scope.$watch(function ngModelWatch() {
var value = ngModelGet($scope);

Expand Down Expand Up @@ -2293,4 +2297,4 @@ var ngModelOptionsDirective = function() {
}
}]
};
};
};
40 changes: 33 additions & 7 deletions test/ng/directive/inputSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -847,22 +847,48 @@ describe('input', function() {
dealoc(doc);
}));


it('should allow cancelling pending updates', inject(function($timeout) {
it('should allow canceling pending updates', inject(function($timeout) {
compileInput(
'<form name="test">'+
'<input type="text" ng-model="name" name="alias" '+
'ng-model-options="{ debounce: 10000 }" />'+
'</form>');
'<input type="text" ng-model="name" name="alias" '+
'ng-model-options="{ debounce: 10000 }" />');

changeInputValueTo('a');
expect(scope.name).toEqual(undefined);
$timeout.flush(2000);
scope.test.alias.$cancelDebounce();
scope.form.alias.$cancelUpdate();
expect(scope.name).toEqual(undefined);
$timeout.flush(10000);
expect(scope.name).toEqual(undefined);
}));

it('should reset input val if cancelUpdate called during pending update', function() {
compileInput(
'<input type="text" ng-model="name" name="alias" '+
'ng-model-options="{ updateOn: \'blur\' }" />');
scope.$digest();

changeInputValueTo('a');
expect(inputElm.val()).toBe('a');
scope.form.alias.$cancelUpdate();
expect(inputElm.val()).toBe('');
browserTrigger(inputElm, 'blur');
expect(inputElm.val()).toBe('');
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic! I guess that this fails before your fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does.


it('should reset input val if cancelUpdate called during debounce', inject(function($timeout) {
compileInput(
'<input type="text" ng-model="name" name="alias" '+
'ng-model-options="{ debounce: 2000 }" />');
scope.$digest();

changeInputValueTo('a');
expect(inputElm.val()).toBe('a');
scope.form.alias.$cancelUpdate();
expect(inputElm.val()).toBe('');
$timeout.flush(3000);
expect(inputElm.val()).toBe('');
}));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need another unit test that works over the second case, where there is no debounce but an event that has not yet occurred before the programmatic reset occurs.

});

it('should allow complex reference binding', function() {
Expand Down