Skip to content

Commit b745289

Browse files
caitpnetman92
caitp
authored and
netman92
committed
fix($compile): ignore optional =-bound properties with empty value
Previously, optional empty '='-bound expressions were ignored --- erroneously they stopped being ignored, and no tests were caused to fail for this reason. This change restores the original ignoring behaviour while still preventing other errors fixed by 8a1eb16 Closes angular#12144 Closes angular#12259 Closes angular#12290
1 parent b62ddd9 commit b745289

File tree

2 files changed

+152
-27
lines changed

2 files changed

+152
-27
lines changed

‎src/ng/compile.js

+13-15
Original file line numberDiff line numberDiff line change
@@ -2567,36 +2567,33 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
25672567
lastValue,
25682568
parentGet, parentSet, compare;
25692569

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

25782572
case '@':
2579-
if (!attrs[attrName] && !optional) {
2580-
destination[scopeName] = undefined;
2573+
if (!optional && !hasOwnProperty.call(attrs, attrName)) {
2574+
destination[scopeName] = attrs[attrName] = void 0;
25812575
}
2582-
25832576
attrs.$observe(attrName, function(value) {
2584-
destination[scopeName] = value;
2577+
if (isString(value)) {
2578+
destination[scopeName] = value;
2579+
}
25852580
});
25862581
attrs.$$observers[attrName].$$scope = scope;
2587-
if (attrs[attrName]) {
2582+
if (isString(attrs[attrName])) {
25882583
// If the attribute has been provided then we trigger an interpolation to ensure
25892584
// the value is there for use in the link fn
25902585
destination[scopeName] = $interpolate(attrs[attrName])(scope);
25912586
}
25922587
break;
25932588

25942589
case '=':
2595-
if (optional && !attrs[attrName]) {
2596-
return;
2590+
if (!hasOwnProperty.call(attrs, attrName)) {
2591+
if (optional) break;
2592+
attrs[attrName] = void 0;
25972593
}
2598-
parentGet = $parse(attrs[attrName]);
2594+
if (optional && !attrs[attrName]) break;
25992595

2596+
parentGet = $parse(attrs[attrName]);
26002597
if (parentGet.literal) {
26012598
compare = equals;
26022599
} else {
@@ -2635,7 +2632,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
26352632
break;
26362633

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

26402638
// Don't assign noop to destination if expression is not valid
26412639
if (parentGet === noop && optional) break;

‎test/ng/compileSpec.js

+139-12
Original file line numberDiff line numberDiff line change
@@ -2543,10 +2543,17 @@ describe('$compile', function() {
25432543
};
25442544

25452545
expect(func).not.toThrow();
2546-
expect(element.find('span').scope()).toBe(element.isolateScope());
2547-
expect(element.isolateScope()).not.toBe($rootScope);
2548-
expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
2549-
expect(element.isolateScope()['valueOf']).toBeUndefined();
2546+
var scope = element.isolateScope();
2547+
expect(element.find('span').scope()).toBe(scope);
2548+
expect(scope).not.toBe($rootScope);
2549+
2550+
// Not shadowed because optional
2551+
expect(scope.constructor).toBe($rootScope.constructor);
2552+
expect(scope.hasOwnProperty('constructor')).toBe(false);
2553+
2554+
// Shadowed with undefined because not optional
2555+
expect(scope.valueOf).toBeUndefined();
2556+
expect(scope.hasOwnProperty('valueOf')).toBe(true);
25502557
})
25512558
);
25522559

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

25632570
expect(func).not.toThrow();
2564-
expect(element.find('span').scope()).toBe(element.isolateScope());
2565-
expect(element.isolateScope()).not.toBe($rootScope);
2566-
expect(element.isolateScope()['constructor']).toBe('constructor');
2567-
expect(element.isolateScope()['valueOf']).toBe('valueOf');
2571+
var scope = element.isolateScope();
2572+
expect(element.find('span').scope()).toBe(scope);
2573+
expect(scope).not.toBe($rootScope);
2574+
expect(scope.constructor).toBe('constructor');
2575+
expect(scope.hasOwnProperty('constructor')).toBe(true);
2576+
expect(scope.valueOf).toBe('valueOf');
2577+
expect(scope.hasOwnProperty('valueOf')).toBe(true);
25682578
})
25692579
);
25702580

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

25772587
expect(func).not.toThrow();
2578-
expect(element.find('span').scope()).toBe(element.isolateScope());
2579-
expect(element.isolateScope()).not.toBe($rootScope);
2580-
expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
2581-
expect(element.isolateScope()['valueOf']).toBeUndefined();
2588+
var scope = element.isolateScope();
2589+
expect(element.find('span').scope()).toBe(scope);
2590+
expect(scope).not.toBe($rootScope);
2591+
2592+
// Does not shadow value because optional
2593+
expect(scope.constructor).toBe($rootScope.constructor);
2594+
expect(scope.hasOwnProperty('constructor')).toBe(false);
2595+
2596+
// Shadows value because not optional
2597+
expect(scope.valueOf).toBeUndefined();
2598+
expect(scope.hasOwnProperty('valueOf')).toBe(true);
25822599
})
25832600
);
25842601

@@ -3576,6 +3593,58 @@ describe('$compile', function() {
35763593
}));
35773594

35783595

3596+
it('should not overwrite @-bound property each digest when not present', function() {
3597+
module(function($compileProvider) {
3598+
$compileProvider.directive('testDir', valueFn({
3599+
scope: {prop: '@'},
3600+
controller: function($scope) {
3601+
$scope.prop = $scope.prop || 'default';
3602+
this.getProp = function() {
3603+
return $scope.prop;
3604+
};
3605+
},
3606+
controllerAs: 'ctrl',
3607+
template: '<p></p>'
3608+
}));
3609+
});
3610+
inject(function($compile, $rootScope) {
3611+
element = $compile('<div test-dir></div>')($rootScope);
3612+
var scope = element.isolateScope();
3613+
expect(scope.ctrl.getProp()).toBe('default');
3614+
3615+
$rootScope.$digest();
3616+
expect(scope.ctrl.getProp()).toBe('default');
3617+
});
3618+
});
3619+
3620+
3621+
it('should ignore optional "="-bound property if value is the emptry string', function() {
3622+
module(function($compileProvider) {
3623+
$compileProvider.directive('testDir', valueFn({
3624+
scope: {prop: '=?'},
3625+
controller: function($scope) {
3626+
$scope.prop = $scope.prop || 'default';
3627+
this.getProp = function() {
3628+
return $scope.prop;
3629+
};
3630+
},
3631+
controllerAs: 'ctrl',
3632+
template: '<p></p>'
3633+
}));
3634+
});
3635+
inject(function($compile, $rootScope) {
3636+
element = $compile('<div test-dir></div>')($rootScope);
3637+
var scope = element.isolateScope();
3638+
expect(scope.ctrl.getProp()).toBe('default');
3639+
$rootScope.$digest();
3640+
expect(scope.ctrl.getProp()).toBe('default');
3641+
scope.prop = 'foop';
3642+
$rootScope.$digest();
3643+
expect(scope.ctrl.getProp()).toBe('foop');
3644+
});
3645+
});
3646+
3647+
35793648
describe('bind-once', function() {
35803649

35813650
function countWatches(scope) {
@@ -4437,6 +4506,64 @@ describe('$compile', function() {
44374506
childScope.theCtrl.test();
44384507
});
44394508
});
4509+
4510+
describe('should not overwrite @-bound property each digest when not present', function() {
4511+
it('when creating new scope', function() {
4512+
module(function($compileProvider) {
4513+
$compileProvider.directive('testDir', valueFn({
4514+
scope: true,
4515+
bindToController: {
4516+
prop: '@'
4517+
},
4518+
controller: function() {
4519+
var self = this;
4520+
this.prop = this.prop || 'default';
4521+
this.getProp = function() {
4522+
return self.prop;
4523+
};
4524+
},
4525+
controllerAs: 'ctrl',
4526+
template: '<p></p>'
4527+
}));
4528+
});
4529+
inject(function($compile, $rootScope) {
4530+
element = $compile('<div test-dir></div>')($rootScope);
4531+
var scope = element.scope();
4532+
expect(scope.ctrl.getProp()).toBe('default');
4533+
4534+
$rootScope.$digest();
4535+
expect(scope.ctrl.getProp()).toBe('default');
4536+
});
4537+
});
4538+
4539+
it('when creating isolate scope', function() {
4540+
module(function($compileProvider) {
4541+
$compileProvider.directive('testDir', valueFn({
4542+
scope: {},
4543+
bindToController: {
4544+
prop: '@'
4545+
},
4546+
controller: function() {
4547+
var self = this;
4548+
this.prop = this.prop || 'default';
4549+
this.getProp = function() {
4550+
return self.prop;
4551+
};
4552+
},
4553+
controllerAs: 'ctrl',
4554+
template: '<p></p>'
4555+
}));
4556+
});
4557+
inject(function($compile, $rootScope) {
4558+
element = $compile('<div test-dir></div>')($rootScope);
4559+
var scope = element.isolateScope();
4560+
expect(scope.ctrl.getProp()).toBe('default');
4561+
4562+
$rootScope.$digest();
4563+
expect(scope.ctrl.getProp()).toBe('default');
4564+
});
4565+
});
4566+
});
44404567
});
44414568

44424569

0 commit comments

Comments
 (0)