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

fix($rootScope): provide correct value of one-time bindings in watchGroup #15854

Merged
merged 1 commit into from
Mar 31, 2017
Merged

fix($rootScope): provide correct value of one-time bindings in watchGroup #15854

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
19 changes: 12 additions & 7 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,8 @@ function $RootScopeProvider() {
}

forEach(watchExpressions, function(expr, i) {
var unwatchFn = self.$watch(expr, function watchGroupSubAction(value, oldValue) {
var unwatchFn = self.$watch(expr, function watchGroupSubAction(value) {
newValues[i] = value;
oldValues[i] = oldValue;
if (!changeReactionScheduled) {
changeReactionScheduled = true;
self.$evalAsync(watchGroupAction);
Expand All @@ -504,11 +503,17 @@ function $RootScopeProvider() {
function watchGroupAction() {
changeReactionScheduled = false;

if (firstRun) {
firstRun = false;
listener(newValues, newValues, self);
} else {
listener(newValues, oldValues, self);
try {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, we could avoid the try/finally block, by using one extra array. But my intuition is that it won't make any difference.

if (firstRun) {
firstRun = false;
listener(newValues, newValues, self);
} else {
listener(newValues, oldValues, self);
}
} finally {
for (var i = 0; i < watchExpressions.length; i++) {
oldValues[i] = newValues[i];
}
}
}

Expand Down
88 changes: 88 additions & 0 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,94 @@ describe('Scope', function() {
expect(log).toEqual('');
});

it('should remove all watchers once one-time/constant bindings are stable', function() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems unrelated, but there are no other watcher-count tests so I thought I'd leave it in...

//empty
scope.$watchGroup([], noop);
//single one-time
scope.$watchGroup(['::a'], noop);
//multi one-time
scope.$watchGroup(['::a', '::b'], noop);
//single constant
scope.$watchGroup(['1'], noop);
//multi constant
scope.$watchGroup(['1', '2'], noop);

expect(scope.$$watchersCount).not.toBe(0);
scope.$apply('a = b = 1');
expect(scope.$$watchersCount).toBe(0);
});

it('should maintain correct new/old values with one time bindings', function() {
var newValues;
var oldValues;
scope.$watchGroup(['a', '::b', 'b', '4'], function(n, o) {
newValues = n.slice();
oldValues = o.slice();
});

scope.$apply();
expect(newValues).toEqual(oldValues);
expect(oldValues).toEqual([undefined, undefined, undefined, 4]);

scope.$apply('a = 1');
expect(newValues).toEqual([1, undefined, undefined, 4]);
expect(oldValues).toEqual([undefined, undefined, undefined, 4]);

scope.$apply('b = 2');
expect(newValues).toEqual([1, 2, 2, 4]);
expect(oldValues).toEqual([1, undefined, undefined, 4]);

scope.$apply('b = 3');
expect(newValues).toEqual([1, 2, 3, 4]);
expect(oldValues).toEqual([1, 2, 2, 4]);

scope.$apply('b = 4');
expect(newValues).toEqual([1, 2, 4, 4]);
expect(oldValues).toEqual([1, 2, 3, 4]);
});
});

describe('$watchGroup with logging $exceptionHandler', function() {
it('should maintain correct new/old values even when listener throws', function() {
module(function($exceptionHandlerProvider) {
$exceptionHandlerProvider.mode('log');
});

inject(function($rootScope, $exceptionHandler) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider using scope instead of $rootScope for consistency 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've done this in the other tests which already have a beforeEach that sets scope, but this describe block is new... worth adding a beforeEach when there's only one test?

var newValues;
var oldValues;
$rootScope.$watchGroup(['a', '::b', 'b', '4'], function(n, o) {
newValues = n.slice();
oldValues = o.slice();
throw 'test';
});

$rootScope.$apply();
expect(newValues).toEqual(oldValues);
expect(oldValues).toEqual([undefined, undefined, undefined, 4]);
expect($exceptionHandler.errors.length).toBe(1);

$rootScope.$apply('a = 1');
expect(newValues).toEqual([1, undefined, undefined, 4]);
expect(oldValues).toEqual([undefined, undefined, undefined, 4]);
expect($exceptionHandler.errors.length).toBe(2);

$rootScope.$apply('b = 2');
expect(newValues).toEqual([1, 2, 2, 4]);
expect(oldValues).toEqual([1, undefined, undefined, 4]);
expect($exceptionHandler.errors.length).toBe(3);

$rootScope.$apply('b = 3');
expect(newValues).toEqual([1, 2, 3, 4]);
expect(oldValues).toEqual([1, 2, 2, 4]);
expect($exceptionHandler.errors.length).toBe(4);

$rootScope.$apply('b = 4');
expect(newValues).toEqual([1, 2, 4, 4]);
expect(oldValues).toEqual([1, 2, 3, 4]);
expect($exceptionHandler.errors.length).toBe(5);
});
});
});

describe('$destroy', function() {
Expand Down