fix($rootScope): provide correct value of one-time bindings in watchGroup #15854
Conversation
| @@ -1097,6 +1097,94 @@ describe('Scope', function() { | |||
| expect(log).toEqual(''); | |||
| }); | |||
|
|
|||
| it('should remove all watchers once one-time/constant bindings are stable', function() { | |||
There was a problem hiding this comment.
This seems unrelated, but there are no other watcher-count tests so I thought I'd leave it in...
There was a problem hiding this comment.
This looks useful. I wonder if there are performance implications with the new try ... catch block? I vaguely remember an issue where a try / catch block was not correctly optimized in V8, I think.
But I assume it's not performance critical as it is in the watchGroupAction fn
test/ng/rootScopeSpec.js
Outdated
| })); | ||
| }); | ||
|
|
||
| describe('$watchGroup'/*with logging $exceptionHandler*/, function() { |
There was a problem hiding this comment.
Why is part of this commented out?
There was a problem hiding this comment.
idk, I guess I thought it was too long. I'll uncomment it...
|
The try/catch only wraps the listener, not anything invoked per digest, so I wouldn't be too worried about it. The method is also quite simply and mainly just calls another method, so if that one method isn't optimized I don't think it's a big deal. |
| listener(newValues, newValues, self); | ||
| } else { | ||
| listener(newValues, oldValues, self); | ||
| try { |
There was a problem hiding this comment.
FWIW, we could avoid the try/finally block, by using one extra array. But my intuition is that it won't make any difference.
test/ng/rootScopeSpec.js
Outdated
| expect(scope.$$watchersCount).toBe(0); | ||
| }); | ||
|
|
||
| it('should maintain correct new/old values with one time bindings', inject(function($rootScope, $exceptionHandler) { |
There was a problem hiding this comment.
$exceptionHandler is not used. And you could probably use scope instead of $rootScope to avoid inject() altogether.
| $exceptionHandlerProvider.mode('log'); | ||
| }); | ||
|
|
||
| inject(function($rootScope, $exceptionHandler) { |
There was a problem hiding this comment.
Consider using scope instead of $rootScope for consistency 😁
There was a problem hiding this comment.
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?
|
Updated the tests. Will merge later today... |
I don't recall how or why I came across this issue, and it's confusing me, but I found this stashed and the tests do make sense...?