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

fix($compile): ensure that $onChanges hook is called correctly #14355

Closed
wants to merge 1 commit into from
Closed

fix($compile): ensure that $onChanges hook is called correctly #14355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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();
Expand All @@ -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);
}
}
Expand All @@ -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();
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 '=':
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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]();
}
}
};
}
Expand Down
82 changes: 66 additions & 16 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3668,17 +3668,25 @@ describe('$compile', function() {
// Setup the directive with two bindings
element = $compile('<c1 prop1="val" prop2="val2" other="val3" attr="{{val4}}"></c1>')($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})
}
]);

Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -3766,9 +3776,9 @@ describe('$compile', function() {

module('my');
inject(function($compile, $rootScope) {
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
expect(log).toEqual([]);

$rootScope.$apply('a = 7');
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);
expect(log).toEqual([
{
prop: jasmine.objectContaining({currentValue: 7}),
Expand All @@ -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('<c1 prop="a" attr="{{a}}"></c1>')($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() { }
Expand Down Expand Up @@ -3819,8 +3868,8 @@ describe('$compile', function() {
// Setup two sibling components with bindings that will change
element = $compile('<div><c1 prop="val1"></c1><c2 prop="val2"></c2></div>')($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');
Expand All @@ -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() { }
Expand All @@ -3864,16 +3915,15 @@ describe('$compile', function() {
// Setup the directive with two bindings
element = $compile('<outer prop1="a"></outer>')($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})}]
]);
});
});
Expand Down