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

Commit 0ad2b70

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 #14355 Closes #14359
1 parent 5261a37 commit 0ad2b70

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
@@ -2372,7 +2372,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
23722372

23732373
function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) {
23742374
var i, ii, linkFn, isolateScope, controllerScope, elementControllers, transcludeFn, $element,
2375-
attrs, removeScopeBindingWatches, removeControllerBindingWatches;
2375+
attrs, scopeBindingInfo;
23762376

23772377
if (compileNode === linkNode) {
23782378
attrs = templateAttrs;
@@ -2411,11 +2411,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24112411
compile.$$addScopeClass($element, true);
24122412
isolateScope.$$isolateBindings =
24132413
newIsolateScopeDirective.$$isolateBindings;
2414-
removeScopeBindingWatches = initializeDirectiveBindings(scope, attrs, isolateScope,
2414+
scopeBindingInfo = initializeDirectiveBindings(scope, attrs, isolateScope,
24152415
isolateScope.$$isolateBindings,
24162416
newIsolateScopeDirective);
2417-
if (removeScopeBindingWatches) {
2418-
isolateScope.$on('$destroy', removeScopeBindingWatches);
2417+
if (scopeBindingInfo.removeWatches) {
2418+
isolateScope.$on('$destroy', scopeBindingInfo.removeWatches);
24192419
}
24202420
}
24212421

@@ -2426,8 +2426,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24262426
var bindings = controllerDirective.$$bindings.bindToController;
24272427

24282428
if (controller.identifier && bindings) {
2429-
removeControllerBindingWatches =
2429+
controller.bindingInfo =
24302430
initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective);
2431+
} else {
2432+
controller.bindingInfo = {};
24312433
}
24322434

24332435
var controllerResult = controller();
@@ -2436,8 +2438,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24362438
// from setupControllers
24372439
controller.instance = controllerResult;
24382440
$element.data('$' + controllerDirective.name + 'Controller', controllerResult);
2439-
removeControllerBindingWatches && removeControllerBindingWatches();
2440-
removeControllerBindingWatches =
2441+
controller.bindingInfo.removeWatches && controller.bindingInfo.removeWatches();
2442+
controller.bindingInfo =
24412443
initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective);
24422444
}
24432445
}
@@ -2453,6 +2455,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
24532455
// Handle the init and destroy lifecycle hooks on all controllers that have them
24542456
forEach(elementControllers, function(controller) {
24552457
var controllerInstance = controller.instance;
2458+
if (isFunction(controllerInstance.$onChanges)) {
2459+
controllerInstance.$onChanges(controller.bindingInfo.initialChanges);
2460+
}
24562461
if (isFunction(controllerInstance.$onInit)) {
24572462
controllerInstance.$onInit();
24582463
}
@@ -3099,6 +3104,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
30993104
// only occurs for isolate scopes and new scopes with controllerAs.
31003105
function initializeDirectiveBindings(scope, attrs, destination, bindings, directive) {
31013106
var removeWatchCollection = [];
3107+
var initialChanges = {};
31023108
var changes;
31033109
forEach(bindings, function initializeBinding(definition, scopeName) {
31043110
var attrName = definition.attrName,
@@ -3131,7 +3137,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
31313137
// the value to boolean rather than a string, so we special case this situation
31323138
destination[scopeName] = lastValue;
31333139
}
3134-
recordChanges(scopeName, destination[scopeName], _UNINITIALIZED_VALUE);
3140+
initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]);
31353141
break;
31363142

31373143
case '=':
@@ -3187,7 +3193,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
31873193
parentGet = $parse(attrs[attrName]);
31883194

31893195
destination[scopeName] = parentGet(scope);
3190-
recordChanges(scopeName, destination[scopeName], _UNINITIALIZED_VALUE);
3196+
initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]);
31913197

31923198
removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newParentValue) {
31933199
var oldValue = destination[scopeName];
@@ -3239,9 +3245,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
32393245
changes = undefined;
32403246
}
32413247

3242-
return removeWatchCollection.length && function removeWatches() {
3243-
for (var i = 0, ii = removeWatchCollection.length; i < ii; ++i) {
3244-
removeWatchCollection[i]();
3248+
return {
3249+
initialChanges: initialChanges,
3250+
removeWatches: removeWatchCollection.length && function removeWatches() {
3251+
for (var i = 0, ii = removeWatchCollection.length; i < ii; ++i) {
3252+
removeWatchCollection[i]();
3253+
}
32453254
}
32463255
};
32473256
}

Diff for: ‎test/ng/compileSpec.js

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

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

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

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

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

37393749
// Update val to trigger the onChanges
37403750
$rootScope.$apply('a = 42');
@@ -3765,9 +3775,9 @@ describe('$compile', function() {
37653775

37663776
module('my');
37673777
inject(function($compile, $rootScope) {
3768-
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
3769-
expect(log).toEqual([]);
3778+
37703779
$rootScope.$apply('a = 7');
3780+
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
37713781
expect(log).toEqual([
37723782
{
37733783
prop: jasmine.objectContaining({currentValue: 7}),
@@ -3791,6 +3801,45 @@ describe('$compile', function() {
37913801
});
37923802

37933803

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

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

38243873
// Update val to trigger the onChanges
38253874
$rootScope.$apply('val1 = 42; val2 = 17');
@@ -3836,11 +3885,13 @@ describe('$compile', function() {
38363885

38373886
it('should cope with changes occuring inside `$onChanges()` hooks', function() {
38383887
var log = [];
3839-
function OuterController() { }
3888+
function OuterController() {
3889+
this.prop1 = 0;
3890+
}
38403891
OuterController.prototype.$onChanges = function(change) {
38413892
log.push(['OuterController', change]);
38423893
// Make a change to the inner component
3843-
this.b = 72;
3894+
this.b = this.prop1 * 2;
38443895
};
38453896

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

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

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

38723923
expect(log).toEqual([
38733924
['OuterController', {prop1: jasmine.objectContaining({currentValue: 42})}],
3874-
['InnerController', {prop2: jasmine.objectContaining({currentValue: undefined})}],
3875-
['InnerController', {prop2: jasmine.objectContaining({currentValue: 72})}]
3925+
['InnerController', {prop2: jasmine.objectContaining({currentValue: 84})}]
38763926
]);
38773927
});
38783928
});

0 commit comments

Comments
 (0)