-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix($parse): handle constants as one-time binding expressions #7970
Conversation
Handle constant expressions as one-time binding expressions. Avoids the infinite digest from https://github.com/angular/angular.js/pull/7960/files#r14136938
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! |
Lgtm |
|
||
expect($rootScope.$$watchers.length).toEqual(0); | ||
})); | ||
|
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.
Maybe objects/arrays containing references to scope should be tested so that we don't lose bindings to them? Or is it already covered?
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.
"references to scope"? can you provide an example?
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.
Something along the lines:
var handledFoo;
$rootScope.foo = 'bar';
$rootScope.$watch('[2, foo, "c"]', function(value) {
handledFoo = value[1];
}, true);
$rootScope.$digest();
expect(handledFoo).toBe('bar');
$rootScope.foo = 'baz';
$rootScope.$digest();
expect(handledFoo).toBe('baz');
I guess other tests can be already checking that.
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.
Isn't that really testing if $parse
reports [2, foo, "c"]
as a constant or not?
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.
@rodyhaddad Yes, basically.
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.
@mzgol you cannot watch a non-constant literal expression without specifying objectEquality = true
without triggering a digest issue. This was the case before the one-time binding patch and now
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.
ok
LGTM |
Handle constant expressions as one-time binding expressions. Avoids the infinite digest from https://github.com/angular/angular.js/pull/7960/files#r14136938 Closes angular#7970
Handle constant expressions as one-time binding expressions.
Avoids the infinite digest from
https://github.com/angular/angular.js/pull/7960/files#r14136938