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

fix($compile): don't run unnecessary update to one-way bindings #14580

Merged
merged 1 commit into from
May 10, 2016
Merged

fix($compile): don't run unnecessary update to one-way bindings #14580

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
9 changes: 4 additions & 5 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3213,14 +3213,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

parentGet = $parse(attrs[attrName]);

destination[scopeName] = parentGet(scope);
var initialValue = destination[scopeName] = parentGet(scope);
initialChanges[scopeName] = new SimpleChange(_UNINITIALIZED_VALUE, destination[scopeName]);

removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newValue, oldValue) {
if (newValue === oldValue) {
// If the new and old values are identical then this is the first time the watch has been triggered
// So instead we use the current value on the destination as the old value
oldValue = destination[scopeName];
if (oldValue === newValue) {
if (oldValue === initialValue) return;
oldValue = initialValue;
}
recordChanges(scopeName, newValue, oldValue);
destination[scopeName] = newValue;
Expand Down
111 changes: 105 additions & 6 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3823,6 +3823,7 @@ describe('$compile', function() {

$rootScope.$apply('a = 7');
element = $compile('<c1 prop="a" attr="{{a}}"></c1>')($rootScope);

expect(log).toEqual([
{
prop: jasmine.objectContaining({currentValue: 7}),
Expand Down Expand Up @@ -3862,6 +3863,7 @@ describe('$compile', function() {
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}),
Expand Down Expand Up @@ -4744,6 +4746,7 @@ describe('$compile', function() {
describe('one-way binding', function() {
it('should update isolate when the identity of origin changes', inject(function() {
compile('<div><span my-component ow-ref="obj">');

expect(componentScope.owRef).toBeUndefined();
expect(componentScope.owRefAlias).toBe(componentScope.owRef);

Expand Down Expand Up @@ -4780,6 +4783,7 @@ describe('$compile', function() {

it('should update isolate when both change', inject(function() {
compile('<div><span my-component ow-ref="name">');

$rootScope.name = {mark:123};
componentScope.owRef = 'misko';

Expand All @@ -4796,6 +4800,101 @@ describe('$compile', function() {
expect(componentScope.owRefAlias).toBe($rootScope.name);
}));

describe('initialization', function() {
var component, log;

beforeEach(function() {
log = [];
angular.module('owComponentTest', [])
.component('owComponent', {
bindings: { input: '<' },
controller: function() {
component = this;
this.input = 'constructor';
log.push('constructor');

this.$onInit = function() {
this.input = '$onInit';
log.push('$onInit');
};

this.$onChanges = function(changes) {
if (changes.input) {
log.push(['$onChanges', changes.input]);
}
};
}
});
});

it('should not update isolate again after $onInit if outer has not changed', function() {
module('owComponentTest');
inject(function() {
$rootScope.name = 'outer';
compile('<ow-component input="name"></ow-component>');

expect($rootScope.name).toEqual('outer');
expect(component.input).toEqual('$onInit');

$rootScope.$apply();

expect($rootScope.name).toEqual('outer');
expect(component.input).toEqual('$onInit');

expect(log).toEqual([
'constructor',
['$onChanges', jasmine.objectContaining({ currentValue: 'outer' })],
'$onInit'
]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this test pass even without the fix ? (I haven't tested it, but I think so.)
I think it's a good idea to add a $rootScope.$digest() and another round of expectation to make sure that nothing is overwritten when the watch callback is executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. It did fail before the fix. But then I might have changed it again since then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It totally fails as it is without the fix. But I will add the extra digest and check

});
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent number of newlines between tests 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shock horror! Isn't there a JSCS rule for that??

it('should update isolate again after $onInit if outer has changed (before initial watchAction call)', function() {
module('owComponentTest');
inject(function() {
$rootScope.name = 'outer1';
compile('<ow-component input="name"></ow-component>');

expect(component.input).toEqual('$onInit');
$rootScope.$apply('name = "outer2"');

expect($rootScope.name).toEqual('outer2');
expect(component.input).toEqual('outer2');
expect(log).toEqual([
'constructor',
['$onChanges', jasmine.objectContaining({ currentValue: 'outer1' })],
'$onInit',
['$onChanges', jasmine.objectContaining({ currentValue: 'outer2', previousValue: 'outer1' })]
]);
});
});

it('should update isolate again after $onInit if outer has changed (before initial watchAction call)', function() {
angular.module('owComponentTest')
.directive('changeInput', function() {
return function(scope, elem, attrs) {
scope.name = 'outer2';
};
});
module('owComponentTest');
inject(function() {
$rootScope.name = 'outer1';
compile('<ow-component input="name" change-input></ow-component>');

expect(component.input).toEqual('$onInit');
$rootScope.$digest();

expect($rootScope.name).toEqual('outer2');
expect(component.input).toEqual('outer2');
expect(log).toEqual([
'constructor',
['$onChanges', jasmine.objectContaining({ currentValue: 'outer1' })],
'$onInit',
['$onChanges', jasmine.objectContaining({ currentValue: 'outer2', previousValue: 'outer1' })]
]);
});
});
});

it('should not break when isolate and origin both change to the same value', inject(function() {
$rootScope.name = 'aaa';
Expand All @@ -4819,7 +4918,6 @@ describe('$compile', function() {
$rootScope.name = {mark:123};
compile('<div><span my-component ow-ref="name">');

$rootScope.$apply();
expect($rootScope.name).toEqual({mark:123});
expect(componentScope.owRef).toBe($rootScope.name);
expect(componentScope.owRefAlias).toBe($rootScope.name);
Expand All @@ -4836,7 +4934,6 @@ describe('$compile', function() {
$rootScope.obj = {mark:123};
compile('<div><span my-component ow-ref="obj">');

$rootScope.$apply();
expect($rootScope.obj).toEqual({mark:123});
expect(componentScope.owRef).toBe($rootScope.obj);

Expand All @@ -4849,6 +4946,7 @@ describe('$compile', function() {

it('should not throw on non assignable expressions in the parent', inject(function() {
compile('<div><span my-component ow-ref="\'hello \' + name">');

$rootScope.name = 'world';
$rootScope.$apply();
expect(componentScope.owRef).toBe('hello world');
Expand All @@ -4865,7 +4963,7 @@ describe('$compile', function() {

it('should not throw when assigning to undefined', inject(function() {
compile('<div><span my-component>');
$rootScope.$apply();

expect(componentScope.owRef).toBeUndefined();

componentScope.owRef = 'ignore me';
Expand All @@ -4879,6 +4977,7 @@ describe('$compile', function() {
it('should update isolate scope when "<"-bound NaN changes', inject(function() {
$rootScope.num = NaN;
compile('<div my-component ow-ref="num"></div>');

var isolateScope = element.isolateScope();
expect(isolateScope.owRef).toBeNaN();

Expand Down Expand Up @@ -4917,7 +5016,7 @@ describe('$compile', function() {
$rootScope.name = 'georgios';
$rootScope.obj = {name: 'pete'};
compile('<div><span my-component ow-ref="[{name: name}, obj]">');
$rootScope.$apply();

expect(componentScope.owRef).toEqual([{name: 'georgios'}, {name: 'pete'}]);

$rootScope.name = 'lucas';
Expand All @@ -4931,7 +5030,7 @@ describe('$compile', function() {
$rootScope.name = 'georgios';
$rootScope.obj = {name: 'pete'};
compile('<div><span my-component ow-ref="{name: name, item: obj}">');
$rootScope.$apply();

expect(componentScope.owRef).toEqual({name: 'georgios', item: {name: 'pete'}});

$rootScope.name = 'lucas';
Expand Down Expand Up @@ -4967,7 +5066,6 @@ describe('$compile', function() {
function test(literalString, literalValue) {
compile('<div><span my-component ow-ref="' + literalString + '">');

$rootScope.$apply();
expect(componentScope.owRef).toBe(literalValue);
dealoc(element);
}
Expand All @@ -4976,6 +5074,7 @@ describe('$compile', function() {
describe('optional one-way binding', function() {
it('should update local when origin changes', inject(function() {
compile('<div><span my-component ow-optref="name">');

expect(componentScope.owOptref).toBeUndefined();
expect(componentScope.owOptrefAlias).toBe(componentScope.owOptref);

Expand Down