From 4784a9335e2cd8e0309a99aebbcaa1fc96163cf6 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 31 Mar 2016 22:06:11 +0100 Subject: [PATCH] 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. --- src/ng/compile.js | 33 ++++++++++------- test/ng/compileSpec.js | 82 +++++++++++++++++++++++++++++++++--------- 2 files changed, 87 insertions(+), 28 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index e8eba4f2a820..691891a6901a 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2359,7 +2359,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) { var i, ii, linkFn, isolateScope, controllerScope, elementControllers, transcludeFn, $element, - attrs, removeScopeBindingWatches, removeControllerBindingWatches; + attrs, scopeBindingInfo; if (compileNode === linkNode) { attrs = templateAttrs; @@ -2398,11 +2398,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { compile.$$addScopeClass($element, true); isolateScope.$$isolateBindings = newIsolateScopeDirective.$$isolateBindings; - removeScopeBindingWatches = initializeDirectiveBindings(scope, attrs, isolateScope, + scopeBindingInfo = initializeDirectiveBindings(scope, attrs, isolateScope, isolateScope.$$isolateBindings, newIsolateScopeDirective); - if (removeScopeBindingWatches) { - isolateScope.$on('$destroy', removeScopeBindingWatches); + if (scopeBindingInfo.removeWatches) { + isolateScope.$on('$destroy', scopeBindingInfo.removeWatches); } } @@ -2413,8 +2413,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { var bindings = controllerDirective.$$bindings.bindToController; if (controller.identifier && bindings) { - removeControllerBindingWatches = + controller.bindingInfo = initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective); + } else { + controller.bindingInfo = {}; } var controllerResult = controller(); @@ -2423,8 +2425,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // from setupControllers controller.instance = controllerResult; $element.data('$' + controllerDirective.name + 'Controller', controllerResult); - removeControllerBindingWatches && removeControllerBindingWatches(); - removeControllerBindingWatches = + controller.bindingInfo.removeWatches && controller.bindingInfo.removeWatches(); + controller.bindingInfo = initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective); } } @@ -2440,6 +2442,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // Handle the init and destroy lifecycle hooks on all controllers that have them forEach(elementControllers, function(controller) { var controllerInstance = controller.instance; + if (isFunction(controllerInstance.$onChanges)) { + controllerInstance.$onChanges(controller.bindingInfo.initialChanges); + } if (isFunction(controllerInstance.$onInit)) { controllerInstance.$onInit(); } @@ -3086,6 +3091,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // only occurs for isolate scopes and new scopes with controllerAs. function initializeDirectiveBindings(scope, attrs, destination, bindings, directive) { var removeWatchCollection = []; + var initialChanges = {}; var changes; forEach(bindings, function initializeBinding(definition, scopeName) { var attrName = definition.attrName, @@ -3118,7 +3124,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // the value to boolean rather than a string, so we special case this situation destination[scopeName] = lastValue; } - recordChanges(scopeName, destination[scopeName], _UNINITIALIZED_VALUE); + initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]); break; case '=': @@ -3174,7 +3180,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { parentGet = $parse(attrs[attrName]); destination[scopeName] = parentGet(scope); - recordChanges(scopeName, destination[scopeName], _UNINITIALIZED_VALUE); + initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]); removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newParentValue) { var oldValue = destination[scopeName]; @@ -3226,9 +3232,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { changes = undefined; } - return removeWatchCollection.length && function removeWatches() { - for (var i = 0, ii = removeWatchCollection.length; i < ii; ++i) { - removeWatchCollection[i](); + return { + initialChanges: initialChanges, + removeWatches: removeWatchCollection.length && function removeWatches() { + for (var i = 0, ii = removeWatchCollection.length; i < ii; ++i) { + removeWatchCollection[i](); + } } }; } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 4f37dc345ec6..4ec4b16e3fcb 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3668,17 +3668,25 @@ describe('$compile', function() { // Setup the directive with two bindings element = $compile('')($rootScope); - // There should be no changes initially - expect(log).toEqual([]); + expect(log).toEqual([ + { + prop1: jasmine.objectContaining({currentValue: undefined}), + prop2: jasmine.objectContaining({currentValue: undefined}), + attr: jasmine.objectContaining({currentValue: ''}) + } + ]); + + // Clear the initial changes from the log + log = []; // Update val to trigger the onChanges $rootScope.$apply('val = 42'); + // Now we should have a single changes entry in the log expect(log).toEqual([ { prop1: jasmine.objectContaining({currentValue: 42}), - prop2: jasmine.objectContaining({currentValue: 84}), - attr: jasmine.objectContaining({currentValue: ''}) + prop2: jasmine.objectContaining({currentValue: 84}) } ]); @@ -3734,8 +3742,10 @@ describe('$compile', function() { // therefore triggering the thing that this test is hoping to enfore $rootScope.$watch('a', function(val) { $rootScope.b = val * 2; }); - // There should be no changes initially - expect(log).toEqual([]); + expect(log).toEqual([{prop: jasmine.objectContaining({currentValue: undefined})}]); + + // Clear the initial values from the log + log = []; // Update val to trigger the onChanges $rootScope.$apply('a = 42'); @@ -3766,9 +3776,9 @@ describe('$compile', function() { module('my'); inject(function($compile, $rootScope) { - element = $compile('')($rootScope); - expect(log).toEqual([]); + $rootScope.$apply('a = 7'); + element = $compile('')($rootScope); expect(log).toEqual([ { prop: jasmine.objectContaining({currentValue: 7}), @@ -3792,6 +3802,45 @@ describe('$compile', function() { }); + it('should trigger an initial onChanges call for each binding even if the hook is defined in the constructor', function() { + var log = []; + function TestController() { + this.$onChanges = function(change) { log.push(change); }; + } + + angular.module('my', []) + .component('c1', { + controller: TestController, + bindings: { 'prop': '<', attr: '@' } + }); + + module('my'); + inject(function($compile, $rootScope) { + $rootScope.$apply('a = 7'); + element = $compile('')($rootScope); + expect(log).toEqual([ + { + prop: jasmine.objectContaining({currentValue: 7}), + attr: jasmine.objectContaining({currentValue: '7'}) + } + ]); + expect(log[0].prop.isFirstChange()).toEqual(true); + expect(log[0].attr.isFirstChange()).toEqual(true); + + log = []; + $rootScope.$apply('a = 10'); + expect(log).toEqual([ + { + prop: jasmine.objectContaining({previousValue: 7, currentValue: 10}), + attr: jasmine.objectContaining({previousValue: '7', currentValue: '10'}) + } + ]); + expect(log[0].prop.isFirstChange()).toEqual(false); + expect(log[0].attr.isFirstChange()).toEqual(false); + }); + }); + + it('should only trigger one extra digest however many controllers have changes', function() { var log = []; function TestController1() { } @@ -3819,8 +3868,8 @@ describe('$compile', function() { // Setup two sibling components with bindings that will change element = $compile('
')($rootScope); - // There should be no changes initially - expect(log).toEqual([]); + // Clear out initial changes + log = []; // Update val to trigger the onChanges $rootScope.$apply('val1 = 42; val2 = 17'); @@ -3837,11 +3886,13 @@ describe('$compile', function() { it('should cope with changes occuring inside `$onChanges()` hooks', function() { var log = []; - function OuterController() { } + function OuterController() { + this.prop1 = 0; + } OuterController.prototype.$onChanges = function(change) { log.push(['OuterController', change]); // Make a change to the inner component - this.b = 72; + this.b = this.prop1 * 2; }; function InnerController() { } @@ -3864,16 +3915,15 @@ describe('$compile', function() { // Setup the directive with two bindings element = $compile('')($rootScope); - // There should be no changes initially - expect(log).toEqual([]); + // Clear out initial changes + log = []; // Update val to trigger the onChanges $rootScope.$apply('a = 42'); expect(log).toEqual([ ['OuterController', {prop1: jasmine.objectContaining({currentValue: 42})}], - ['InnerController', {prop2: jasmine.objectContaining({currentValue: undefined})}], - ['InnerController', {prop2: jasmine.objectContaining({currentValue: 72})}] + ['InnerController', {prop2: jasmine.objectContaining({currentValue: 84})}] ]); }); });