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

fix($compile): ignore optional =-bound properties with empty value #12290

Closed
wants to merge 2 commits into from
Closed

fix($compile): ignore optional =-bound properties with empty value #12290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69f1ebd
revert "revert: "fix($compile): do not write @-bound properties if at…
caitp Jul 7, 2015
df73233
fix($compile): ignore optional =-bound properties with empty value
caitp Jul 7, 2015
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
28 changes: 13 additions & 15 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2567,36 +2567,33 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
lastValue,
parentGet, parentSet, compare;

if (!hasOwnProperty.call(attrs, attrName)) {
// In the case of user defined a binding with the same name as a method in Object.prototype but didn't set
// the corresponding attribute. We need to make sure subsequent code won't access to the prototype function
attrs[attrName] = undefined;
}

switch (mode) {

case '@':
if (!attrs[attrName] && !optional) {
destination[scopeName] = undefined;
if (!optional && !hasOwnProperty.call(attrs, attrName)) {
destination[scopeName] = attrs[attrName] = void 0;
}

attrs.$observe(attrName, function(value) {
destination[scopeName] = value;
if (isString(value)) {
destination[scopeName] = value;
}
});
attrs.$$observers[attrName].$$scope = scope;
if (attrs[attrName]) {
if (isString(attrs[attrName])) {
// If the attribute has been provided then we trigger an interpolation to ensure
// the value is there for use in the link fn
destination[scopeName] = $interpolate(attrs[attrName])(scope);
}
break;

case '=':
if (optional && !attrs[attrName]) {
return;
if (!hasOwnProperty.call(attrs, attrName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use attrs.hasOwnProperty (instead of hasOwnProperty.call(attrs) here and above ?
(It is being used below 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.

attrs shouldn't share [[Prototype]] with Object, there are a lot of bugs which have been fixed because of this. the code below just came from a revert of a revert, so it's part of the pieces that haven't been fixed yet

Copy link

Choose a reason for hiding this comment

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

@caitp - in #12144 (comment) you mentioned some additional code required for Angular Material.

  • Is that still true?
  • If yes, should we modify the Angular Material directives with optional 2-way binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code in that snippet is contained in the second commit here, just organized slightly differently --- I decided to avoid the isString() check and just test if it's falsy (values in attrs should always be a string, unless it gets shadowed above, in which case it's undefined --- so also falsy) --- so the simpler version in this PR should be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, the example snippet in that issue also handles (ignores) values which are exclusively whitespace, but I think these are pretty rare

Copy link
Contributor

Choose a reason for hiding this comment

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

@ThomasBurleson - see @caitp's reply to your question, that the code snippet that is mentioned in the previously linked comment is included in this PR. In other words you shouldn't need to make changes to Material for all your (non-disabled) tests to pass.

I think the only thing we can do now is see if this PR works for the latest on Material's master branch and if so then merge this PR.

Copy link

Choose a reason for hiding this comment

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

@petebacondarwin - Re: ... see if this PR works for the latest on Material's master branch. Is this something I should be doing now; or is someone else assigned this task ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ThomasBurleson would be nice if you could test that!

Copy link

Choose a reason for hiding this comment

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

@Narretz - I will test tonight and respond tomorrow. Will that work ?

if (optional) break;
attrs[attrName] = void 0;
}
parentGet = $parse(attrs[attrName]);
if (optional && !attrs[attrName]) break;

parentGet = $parse(attrs[attrName]);
if (parentGet.literal) {
compare = equals;
} else {
Expand Down Expand Up @@ -2635,7 +2632,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
break;

case '&':
parentGet = $parse(attrs[attrName]);
// Don't assign Object.prototype method to scope
parentGet = attrs.hasOwnProperty(attrName) ? $parse(attrs[attrName]) : noop;

// Don't assign noop to destination if expression is not valid
if (parentGet === noop && optional) break;
Expand Down
151 changes: 139 additions & 12 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2543,10 +2543,17 @@ describe('$compile', function() {
};

expect(func).not.toThrow();
expect(element.find('span').scope()).toBe(element.isolateScope());
expect(element.isolateScope()).not.toBe($rootScope);
expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
expect(element.isolateScope()['valueOf']).toBeUndefined();
var scope = element.isolateScope();
expect(element.find('span').scope()).toBe(scope);
expect(scope).not.toBe($rootScope);

// Not shadowed because optional
expect(scope.constructor).toBe($rootScope.constructor);
expect(scope.hasOwnProperty('constructor')).toBe(false);

// Shadowed with undefined because not optional
expect(scope.valueOf).toBeUndefined();
expect(scope.hasOwnProperty('valueOf')).toBe(true);
})
);

Expand All @@ -2561,10 +2568,13 @@ describe('$compile', function() {
};

expect(func).not.toThrow();
expect(element.find('span').scope()).toBe(element.isolateScope());
expect(element.isolateScope()).not.toBe($rootScope);
expect(element.isolateScope()['constructor']).toBe('constructor');
expect(element.isolateScope()['valueOf']).toBe('valueOf');
var scope = element.isolateScope();
expect(element.find('span').scope()).toBe(scope);
expect(scope).not.toBe($rootScope);
expect(scope.constructor).toBe('constructor');
expect(scope.hasOwnProperty('constructor')).toBe(true);
expect(scope.valueOf).toBe('valueOf');
expect(scope.hasOwnProperty('valueOf')).toBe(true);
})
);

Expand All @@ -2575,10 +2585,17 @@ describe('$compile', function() {
};

expect(func).not.toThrow();
expect(element.find('span').scope()).toBe(element.isolateScope());
expect(element.isolateScope()).not.toBe($rootScope);
expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
expect(element.isolateScope()['valueOf']).toBeUndefined();
var scope = element.isolateScope();
expect(element.find('span').scope()).toBe(scope);
expect(scope).not.toBe($rootScope);

// Does not shadow value because optional
expect(scope.constructor).toBe($rootScope.constructor);
expect(scope.hasOwnProperty('constructor')).toBe(false);

// Shadows value because not optional
expect(scope.valueOf).toBeUndefined();
expect(scope.hasOwnProperty('valueOf')).toBe(true);
})
);

Expand Down Expand Up @@ -3576,6 +3593,58 @@ describe('$compile', function() {
}));


it('should not overwrite @-bound property each digest when not present', function() {
module(function($compileProvider) {
$compileProvider.directive('testDir', valueFn({
scope: {prop: '@'},
controller: function($scope) {
$scope.prop = $scope.prop || 'default';
this.getProp = function() {
return $scope.prop;
};
},
controllerAs: 'ctrl',
template: '<p></p>'
}));
});
inject(function($compile, $rootScope) {
element = $compile('<div test-dir></div>')($rootScope);
var scope = element.isolateScope();
expect(scope.ctrl.getProp()).toBe('default');

$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('default');
});
});


it('should ignore optional "="-bound property if value is the emptry string', function() {
module(function($compileProvider) {
$compileProvider.directive('testDir', valueFn({
scope: {prop: '=?'},
controller: function($scope) {
$scope.prop = $scope.prop || 'default';
this.getProp = function() {
return $scope.prop;
};
},
controllerAs: 'ctrl',
template: '<p></p>'
}));
});
inject(function($compile, $rootScope) {
element = $compile('<div test-dir></div>')($rootScope);
var scope = element.isolateScope();
expect(scope.ctrl.getProp()).toBe('default');
$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('default');
scope.prop = 'foop';
$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('foop');
});
});


describe('bind-once', function() {

function countWatches(scope) {
Expand Down Expand Up @@ -4437,6 +4506,64 @@ describe('$compile', function() {
childScope.theCtrl.test();
});
});

describe('should not overwrite @-bound property each digest when not present', function() {
it('when creating new scope', function() {
module(function($compileProvider) {
$compileProvider.directive('testDir', valueFn({
scope: true,
bindToController: {
prop: '@'
},
controller: function() {
var self = this;
this.prop = this.prop || 'default';
this.getProp = function() {
return self.prop;
};
},
controllerAs: 'ctrl',
template: '<p></p>'
}));
});
inject(function($compile, $rootScope) {
element = $compile('<div test-dir></div>')($rootScope);
var scope = element.scope();
expect(scope.ctrl.getProp()).toBe('default');

$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('default');
});
});

it('when creating isolate scope', function() {
module(function($compileProvider) {
$compileProvider.directive('testDir', valueFn({
scope: {},
bindToController: {
prop: '@'
},
controller: function() {
var self = this;
this.prop = this.prop || 'default';
this.getProp = function() {
return self.prop;
};
},
controllerAs: 'ctrl',
template: '<p></p>'
}));
});
inject(function($compile, $rootScope) {
element = $compile('<div test-dir></div>')($rootScope);
var scope = element.isolateScope();
expect(scope.ctrl.getProp()).toBe('default');

$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('default');
});
});
});
});


Expand Down