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

fix(ngOptions): do not watch properties starting with $ #12010

Closed
wants to merge 1 commit into from
Closed

fix(ngOptions): do not watch properties starting with $ #12010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/ng/directive/ngOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
values = values || [];

Object.keys(values).forEach(function getWatchable(key) {
if (key.charAt(0) === '$') return;
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to ignore $ (and not just $$) properties ?
$$ are (by convention) private, but $ are (by convention again) Angular-specific, but not private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkalpak - good question. Right now we are ignoring such properties as options, so there is a precedent: https://github.com/angular/angular.js/blob/master/test/ng/directive/ngOptionsSpec.js#L434-L448

Also angular.equals and ngRepeat ignore single $ too.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. For some reason, ToJsonReplacer() was modifed some time ago to only ignore $$ properties (but everything else (e.g. ngRepeat, ngOptions, equals) kept ignoring $ as well. Too much breaking change I guess :)

So, it's fine as it is 😃

var locals = getLocals(values[key], key);
var selectValue = getTrackByValueFn(values[key], locals);
watchedArray.push(selectValue);
Expand Down
35 changes: 35 additions & 0 deletions test/ng/directive/ngOptionsSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,41 @@ describe('ngOptions', function() {
});


it('should not watch array properties that start with $ or $$', function() {
createSelect({
'ng-options': 'value as createLabel(value) for value in array',
'ng-model': 'selected'
});
scope.createLabel = jasmine.createSpy('createLabel').andCallFake(function(value) { return value; });
scope.array = ['a', 'b', 'c'];
scope.array.$$private = 'do not watch';
scope.array.$property = 'do not watch';
scope.selected = 'b';
scope.$digest();

expect(scope.createLabel).toHaveBeenCalledWith('a');
expect(scope.createLabel).toHaveBeenCalledWith('b');
expect(scope.createLabel).toHaveBeenCalledWith('c');
expect(scope.createLabel).not.toHaveBeenCalledWith('do not watch');
});


it('should not watch object properties that start with $ or $$', function() {
createSelect({
'ng-options': 'key as createLabel(key) for (key, value) in object',
'ng-model': 'selected'
});
scope.createLabel = jasmine.createSpy('createLabel').andCallFake(function(value) { return value; });
scope.object = {'regularProperty': 'visible', '$$private': 'invisible', '$property': 'invisible'};
scope.selected = 'regularProperty';
scope.$digest();

expect(scope.createLabel).toHaveBeenCalledWith('regularProperty');
expect(scope.createLabel).not.toHaveBeenCalledWith('$$private');
expect(scope.createLabel).not.toHaveBeenCalledWith('$property');
});


it('should allow expressions over multiple lines', function() {
scope.isNotFoo = function(item) {
return item.name !== 'Foo';
Expand Down