Skip to content

Commit b54634d

Browse files
petebacondarwinpetebacondarwin
committed
fix($compile): ensure that $onChanges hook is called correctly
Due to the way that we instantiate controllers, the `$onChanges` hook was not always available at the time we were trying to trigger the initial call to this hook. For instance, if the hook was actually defined inside the constructor function. This commit fixes that but also fixes the fact that the initial call was being made in the postDigest anyway, which was incorrect because the it should have been made before the `$onInit` call. Closes angular#14355 Closes angular#14359
1 parent fa79eaa commit b54634d

File tree

2 files changed

+87
-28
lines changed

2 files changed

+87
-28
lines changed

Diff for: ‎src/ng/compile.js

+21-12
Original file line numberDiff line numberDiff line change
@@ -2359,7 +2359,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
23592359

23602360
function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) {
23612361
var i, ii, linkFn, isolateScope, controllerScope, elementControllers, transcludeFn, $element,
2362-
attrs, removeScopeBindingWatches, removeControllerBindingWatches;
2362+
attrs, scopeBindingInfo;
23632363

23642364
if (compileNode === linkNode) {
23652365
attrs = templateAttrs;
@@ -2398,11 +2398,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
23982398
compile.$$addScopeClass($element, true);
23992399
isolateScope.$$isolateBindings =
24002400
newIsolateScopeDirective.$$isolateBindings;
2401-
removeScopeBindingWatches = initializeDirectiveBindings(scope, attrs, isolateScope,
2401+
scopeBindingInfo = initializeDirectiveBindings(scope, attrs, isolateScope,
24022402
isolateScope.$$isolateBindings,
24032403
newIsolateScopeDirective);
2404-
if (removeScopeBindingWatches) {
2405-
isolateScope.$on('$destroy', removeScopeBindingWatches);
2404+
if (scopeBindingInfo.removeWatches) {
2405+
isolateScope.$on('$destroy', scopeBindingInfo.removeWatches);
24062406
}
24072407
}
24082408

@@ -2413,8 +2413,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24132413
var bindings = controllerDirective.$$bindings.bindToController;
24142414

24152415
if (controller.identifier && bindings) {
2416-
removeControllerBindingWatches =
2416+
controller.bindingInfo =
24172417
initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective);
2418+
} else {
2419+
controller.bindingInfo = {};
24182420
}
24192421

24202422
var controllerResult = controller();
@@ -2423,8 +2425,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24232425
// from setupControllers
24242426
controller.instance = controllerResult;
24252427
$element.data('$' + controllerDirective.name + 'Controller', controllerResult);
2426-
removeControllerBindingWatches && removeControllerBindingWatches();
2427-
removeControllerBindingWatches =
2428+
controller.bindingInfo.removeWatches && controller.bindingInfo.removeWatches();
2429+
controller.bindingInfo =
24282430
initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective);
24292431
}
24302432
}
@@ -2440,6 +2442,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24402442
// Handle the init and destroy lifecycle hooks on all controllers that have them
24412443
forEach(elementControllers, function(controller) {
24422444
var controllerInstance = controller.instance;
2445+
if (isFunction(controllerInstance.$onChanges)) {
2446+
controllerInstance.$onChanges(controller.bindingInfo.initialChanges);
2447+
}
24432448
if (isFunction(controllerInstance.$onInit)) {
24442449
controllerInstance.$onInit();
24452450
}
@@ -3086,6 +3091,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
30863091
// only occurs for isolate scopes and new scopes with controllerAs.
30873092
function initializeDirectiveBindings(scope, attrs, destination, bindings, directive) {
30883093
var removeWatchCollection = [];
3094+
var initialChanges = {};
30893095
var changes;
30903096
forEach(bindings, function initializeBinding(definition, scopeName) {
30913097
var attrName = definition.attrName,
@@ -3118,7 +3124,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
31183124
// the value to boolean rather than a string, so we special case this situation
31193125
destination[scopeName] = lastValue;
31203126
}
3121-
recordChanges(scopeName, destination[scopeName], _UNINITIALIZED_VALUE);
3127+
initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]);
31223128
break;
31233129

31243130
case '=':
@@ -3174,7 +3180,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
31743180
parentGet = $parse(attrs[attrName]);
31753181

31763182
destination[scopeName] = parentGet(scope);
3177-
recordChanges(scopeName, destination[scopeName], _UNINITIALIZED_VALUE);
3183+
initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]);
31783184

31793185
removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newParentValue) {
31803186
var oldValue = destination[scopeName];
@@ -3226,9 +3232,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
32263232
changes = undefined;
32273233
}
32283234

3229-
return removeWatchCollection.length && function removeWatches() {
3230-
for (var i = 0, ii = removeWatchCollection.length; i < ii; ++i) {
3231-
removeWatchCollection[i]();
3235+
return {
3236+
initialChanges: initialChanges,
3237+
removeWatches: removeWatchCollection.length && function removeWatches() {
3238+
for (var i = 0, ii = removeWatchCollection.length; i < ii; ++i) {
3239+
removeWatchCollection[i]();
3240+
}
32323241
}
32333242
};
32343243
}

Diff for: ‎test/ng/compileSpec.js

+66-16
Original file line numberDiff line numberDiff line change
@@ -3668,17 +3668,25 @@ describe('$compile', function() {
36683668
// Setup the directive with two bindings
36693669
element = $compile('<c1 prop1="val" prop2="val2" other="val3" attr="{{val4}}"></c1>')($rootScope);
36703670

3671-
// There should be no changes initially
3672-
expect(log).toEqual([]);
3671+
expect(log).toEqual([
3672+
{
3673+
prop1: jasmine.objectContaining({currentValue: undefined}),
3674+
prop2: jasmine.objectContaining({currentValue: undefined}),
3675+
attr: jasmine.objectContaining({currentValue: ''})
3676+
}
3677+
]);
3678+
3679+
// Clear the initial changes from the log
3680+
log = [];
36733681

36743682
// Update val to trigger the onChanges
36753683
$rootScope.$apply('val = 42');
3684+
36763685
// Now we should have a single changes entry in the log
36773686
expect(log).toEqual([
36783687
{
36793688
prop1: jasmine.objectContaining({currentValue: 42}),
3680-
prop2: jasmine.objectContaining({currentValue: 84}),
3681-
attr: jasmine.objectContaining({currentValue: ''})
3689+
prop2: jasmine.objectContaining({currentValue: 84})
36823690
}
36833691
]);
36843692

@@ -3734,8 +3742,10 @@ describe('$compile', function() {
37343742
// therefore triggering the thing that this test is hoping to enfore
37353743
$rootScope.$watch('a', function(val) { $rootScope.b = val * 2; });
37363744

3737-
// There should be no changes initially
3738-
expect(log).toEqual([]);
3745+
expect(log).toEqual([{prop: jasmine.objectContaining({currentValue: undefined})}]);
3746+
3747+
// Clear the initial values from the log
3748+
log = [];
37393749

37403750
// Update val to trigger the onChanges
37413751
$rootScope.$apply('a = 42');
@@ -3766,9 +3776,9 @@ describe('$compile', function() {
37663776

37673777
module('my');
37683778
inject(function($compile, $rootScope) {
3769-
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
3770-
expect(log).toEqual([]);
3779+
37713780
$rootScope.$apply('a = 7');
3781+
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
37723782
expect(log).toEqual([
37733783
{
37743784
prop: jasmine.objectContaining({currentValue: 7}),
@@ -3792,6 +3802,45 @@ describe('$compile', function() {
37923802
});
37933803

37943804

3805+
it('should trigger an initial onChanges call for each binding even if the hook is defined in the constructor', function() {
3806+
var log = [];
3807+
function TestController() {
3808+
this.$onChanges = function(change) { log.push(change); };
3809+
}
3810+
3811+
angular.module('my', [])
3812+
.component('c1', {
3813+
controller: TestController,
3814+
bindings: { 'prop': '<', attr: '@' }
3815+
});
3816+
3817+
module('my');
3818+
inject(function($compile, $rootScope) {
3819+
$rootScope.$apply('a = 7');
3820+
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
3821+
expect(log).toEqual([
3822+
{
3823+
prop: jasmine.objectContaining({currentValue: 7}),
3824+
attr: jasmine.objectContaining({currentValue: '7'})
3825+
}
3826+
]);
3827+
expect(log[0].prop.isFirstChange()).toEqual(true);
3828+
expect(log[0].attr.isFirstChange()).toEqual(true);
3829+
3830+
log = [];
3831+
$rootScope.$apply('a = 10');
3832+
expect(log).toEqual([
3833+
{
3834+
prop: jasmine.objectContaining({previousValue: 7, currentValue: 10}),
3835+
attr: jasmine.objectContaining({previousValue: '7', currentValue: '10'})
3836+
}
3837+
]);
3838+
expect(log[0].prop.isFirstChange()).toEqual(false);
3839+
expect(log[0].attr.isFirstChange()).toEqual(false);
3840+
});
3841+
});
3842+
3843+
37953844
it('should only trigger one extra digest however many controllers have changes', function() {
37963845
var log = [];
37973846
function TestController1() { }
@@ -3819,8 +3868,8 @@ describe('$compile', function() {
38193868
// Setup two sibling components with bindings that will change
38203869
element = $compile('<div><c1 prop="val1"></c1><c2 prop="val2"></c2></div>')($rootScope);
38213870

3822-
// There should be no changes initially
3823-
expect(log).toEqual([]);
3871+
// Clear out initial changes
3872+
log = [];
38243873

38253874
// Update val to trigger the onChanges
38263875
$rootScope.$apply('val1 = 42; val2 = 17');
@@ -3837,11 +3886,13 @@ describe('$compile', function() {
38373886

38383887
it('should cope with changes occuring inside `$onChanges()` hooks', function() {
38393888
var log = [];
3840-
function OuterController() { }
3889+
function OuterController() {
3890+
this.prop1 = 0;
3891+
}
38413892
OuterController.prototype.$onChanges = function(change) {
38423893
log.push(['OuterController', change]);
38433894
// Make a change to the inner component
3844-
this.b = 72;
3895+
this.b = this.prop1 * 2;
38453896
};
38463897

38473898
function InnerController() { }
@@ -3864,16 +3915,15 @@ describe('$compile', function() {
38643915
// Setup the directive with two bindings
38653916
element = $compile('<outer prop1="a"></outer>')($rootScope);
38663917

3867-
// There should be no changes initially
3868-
expect(log).toEqual([]);
3918+
// Clear out initial changes
3919+
log = [];
38693920

38703921
// Update val to trigger the onChanges
38713922
$rootScope.$apply('a = 42');
38723923

38733924
expect(log).toEqual([
38743925
['OuterController', {prop1: jasmine.objectContaining({currentValue: 42})}],
3875-
['InnerController', {prop2: jasmine.objectContaining({currentValue: undefined})}],
3876-
['InnerController', {prop2: jasmine.objectContaining({currentValue: 72})}]
3926+
['InnerController', {prop2: jasmine.objectContaining({currentValue: 84})}]
38773927
]);
38783928
});
38793929
});

0 commit comments

Comments
 (0)