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

Commit 92d022d

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.
1 parent fa79eaa commit 92d022d

File tree

2 files changed

+87
-27
lines changed

2 files changed

+87
-27
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-15
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

@@ -3735,7 +3743,10 @@ describe('$compile', function() {
37353743
$rootScope.$watch('a', function(val) { $rootScope.b = val * 2; });
37363744

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

37403751
// Update val to trigger the onChanges
37413752
$rootScope.$apply('a = 42');
@@ -3766,9 +3777,9 @@ describe('$compile', function() {
37663777

37673778
module('my');
37683779
inject(function($compile, $rootScope) {
3769-
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
3770-
expect(log).toEqual([]);
3780+
37713781
$rootScope.$apply('a = 7');
3782+
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
37723783
expect(log).toEqual([
37733784
{
37743785
prop: jasmine.objectContaining({currentValue: 7}),
@@ -3792,6 +3803,45 @@ describe('$compile', function() {
37923803
});
37933804

37943805

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

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

38253875
// Update val to trigger the onChanges
38263876
$rootScope.$apply('val1 = 42; val2 = 17');
@@ -3837,11 +3887,13 @@ describe('$compile', function() {
38373887

38383888
it('should cope with changes occuring inside `$onChanges()` hooks', function() {
38393889
var log = [];
3840-
function OuterController() { }
3890+
function OuterController() {
3891+
this.prop1 = 0;
3892+
}
38413893
OuterController.prototype.$onChanges = function(change) {
38423894
log.push(['OuterController', change]);
38433895
// Make a change to the inner component
3844-
this.b = 72;
3896+
this.b = this.prop1 * 2;
38453897
};
38463898

38473899
function InnerController() { }
@@ -3864,16 +3916,15 @@ describe('$compile', function() {
38643916
// Setup the directive with two bindings
38653917
element = $compile('<outer prop1="a"></outer>')($rootScope);
38663918

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

38703922
// Update val to trigger the onChanges
38713923
$rootScope.$apply('a = 42');
38723924

38733925
expect(log).toEqual([
38743926
['OuterController', {prop1: jasmine.objectContaining({currentValue: 42})}],
3875-
['InnerController', {prop2: jasmine.objectContaining({currentValue: undefined})}],
3876-
['InnerController', {prop2: jasmine.objectContaining({currentValue: 72})}]
3927+
['InnerController', {prop2: jasmine.objectContaining({currentValue: 84})}]
38773928
]);
38783929
});
38793930
});

0 commit comments

Comments
 (0)