From a58a0522394beec5c237a615f64b4b1131b050d4 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Fri, 24 Mar 2017 01:44:42 -0700 Subject: [PATCH 1/3] fix($parse): standardize one-time literal vs non-literal and interceptors Previously literal one-time bindings did not use the expression `inputs`, causing infinite digest issues with literal values. This often forces the use of deepEquals when watching one-time literals. `ng-class` is one example of deepEquals which is no longer required. This one-time/literal behavior is now also consistently propogated through interceptors. --- src/ng/directive/ngClass.js | 53 ++---------- src/ng/parse.js | 64 ++++++--------- test/ng/parseSpec.js | 156 ++++++++++++++++++------------------ 3 files changed, 114 insertions(+), 159 deletions(-) diff --git a/src/ng/directive/ngClass.js b/src/ng/directive/ngClass.js index a26c1c77187b..440c0b839d2b 100644 --- a/src/ng/directive/ngClass.js +++ b/src/ng/directive/ngClass.js @@ -14,13 +14,6 @@ function classDirective(name, selector) { return { restrict: 'AC', link: function(scope, element, attr) { - var expression = attr[name].trim(); - var isOneTime = (expression.charAt(0) === ':') && (expression.charAt(1) === ':'); - - var watchInterceptor = isOneTime ? toFlatValue : toClassString; - var watchExpression = $parse(expression, watchInterceptor); - var watchAction = isOneTime ? ngClassOneTimeWatchAction : ngClassWatchAction; - var classCounts = element.data('$classCounts'); var oldModulo = true; var oldClassString; @@ -43,7 +36,7 @@ function classDirective(name, selector) { scope.$watch(indexWatchExpression, ngClassIndexWatchAction); } - scope.$watch(watchExpression, watchAction, isOneTime); + scope.$watch($parse(attr[name], toClassString), ngClassWatchAction); function addClasses(classString) { classString = digestClassCounts(split(classString), 1); @@ -85,9 +78,9 @@ function classDirective(name, selector) { } function ngClassIndexWatchAction(newModulo) { - // This watch-action should run before the `ngClass[OneTime]WatchAction()`, thus it + // This watch-action should run before the `ngClassWatchAction()`, thus it // adds/removes `oldClassString`. If the `ngClass` expression has changed as well, the - // `ngClass[OneTime]WatchAction()` will update the classes. + // `ngClassWatchAction()` will update the classes. if (newModulo === selector) { addClasses(oldClassString); } else { @@ -97,15 +90,13 @@ function classDirective(name, selector) { oldModulo = newModulo; } - function ngClassOneTimeWatchAction(newClassValue) { - var newClassString = toClassString(newClassValue); - - if (newClassString !== oldClassString) { - ngClassWatchAction(newClassString); + function ngClassWatchAction(newClassString) { + //When using a one-time binding the newClassString will return + //the pre-interceptor value until the one-time is complete + if (newClassString && typeof newClassString !== "string") { + newClassString = toClassString(newClassString); } - } - function ngClassWatchAction(newClassString) { if (oldModulo === selector) { updateClasses(oldClassString, newClassString); } @@ -152,34 +143,6 @@ function classDirective(name, selector) { return classString; } - - function toFlatValue(classValue) { - var flatValue = classValue; - - if (isArray(classValue)) { - flatValue = classValue.map(toFlatValue); - } else if (isObject(classValue)) { - var hasUndefined = false; - - flatValue = Object.keys(classValue).filter(function(key) { - var value = classValue[key]; - - if (!hasUndefined && isUndefined(value)) { - hasUndefined = true; - } - - return value; - }); - - if (hasUndefined) { - // Prevent the `oneTimeLiteralWatchInterceptor` from unregistering - // the watcher, by including at least one `undefined` value. - flatValue.push(undefined); - } - } - - return flatValue; - } } /** diff --git a/src/ng/parse.js b/src/ng/parse.js index 8956c618fa4c..78ea77e97657 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -1765,8 +1765,8 @@ function $ParseProvider() { if (parsedExpression.constant) { parsedExpression.$$watchDelegate = constantWatchDelegate; } else if (oneTime) { - parsedExpression.$$watchDelegate = parsedExpression.literal ? - oneTimeLiteralWatchDelegate : oneTimeWatchDelegate; + parsedExpression.oneTime = true; + parsedExpression.$$watchDelegate = oneTimeWatchDelegate; } else if (parsedExpression.inputs) { parsedExpression.$$watchDelegate = inputsWatchDelegate; } @@ -1852,6 +1852,7 @@ function $ParseProvider() { } function oneTimeWatchDelegate(scope, listener, objectEquality, parsedExpression, prettyPrintExpression) { + var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined; var unwatch, lastValue; if (parsedExpression.inputs) { unwatch = inputsWatchDelegate(scope, oneTimeListener, objectEquality, parsedExpression, prettyPrintExpression); @@ -1868,9 +1869,9 @@ function $ParseProvider() { if (isFunction(listener)) { listener(value, old, scope); } - if (isDefined(value)) { + if (doneCriteria(value)) { scope.$$postDigest(function() { - if (isDefined(lastValue)) { + if (doneCriteria(lastValue)) { unwatch(); } }); @@ -1878,31 +1879,12 @@ function $ParseProvider() { } } - function oneTimeLiteralWatchDelegate(scope, listener, objectEquality, parsedExpression) { - var unwatch, lastValue; - unwatch = scope.$watch(function oneTimeWatch(scope) { - return parsedExpression(scope); - }, function oneTimeListener(value, old, scope) { - lastValue = value; - if (isFunction(listener)) { - listener(value, old, scope); - } - if (isAllDefined(value)) { - scope.$$postDigest(function() { - if (isAllDefined(lastValue)) unwatch(); - }); - } - }, objectEquality); - - return unwatch; - - function isAllDefined(value) { - var allDefined = true; - forEach(value, function(val) { - if (!isDefined(val)) allDefined = false; - }); - return allDefined; - } + function isAllDefined(value) { + var allDefined = true; + forEach(value, function(val) { + if (!isDefined(val)) allDefined = false; + }); + return allDefined; } function constantWatchDelegate(scope, listener, objectEquality, parsedExpression) { @@ -1918,22 +1900,28 @@ function $ParseProvider() { var watchDelegate = parsedExpression.$$watchDelegate; var useInputs = false; - var regularWatch = - watchDelegate !== oneTimeLiteralWatchDelegate && - watchDelegate !== oneTimeWatchDelegate; + var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined; - var fn = regularWatch ? function regularInterceptedExpression(scope, locals, assign, inputs) { + function regularInterceptedExpression(scope, locals, assign, inputs) { var value = useInputs && inputs ? inputs[0] : parsedExpression(scope, locals, assign, inputs); return interceptorFn(value, scope, locals); - } : function oneTimeInterceptedExpression(scope, locals, assign, inputs) { - var value = parsedExpression(scope, locals, assign, inputs); + } + + function oneTimeInterceptedExpression(scope, locals, assign, inputs) { + var value = useInputs && inputs ? inputs[0] : parsedExpression(scope, locals, assign, inputs); var result = interceptorFn(value, scope, locals); // we only return the interceptor's result if the // initial value is defined (for bind-once) - return isDefined(value) ? result : value; - }; + return doneCriteria(value) ? result : value; + } + + var fn = parsedExpression.oneTime ? oneTimeInterceptedExpression : regularInterceptedExpression; + + // Propogate the literal/oneTime attributes + fn.literal = parsedExpression.literal; + fn.oneTime = parsedExpression.oneTime; - // Propagate $$watchDelegates other then inputsWatchDelegate + // Propagate or create inputs / $$watchDelegates useInputs = !parsedExpression.inputs; if (watchDelegate && watchDelegate !== inputsWatchDelegate) { fn.$$watchDelegate = watchDelegate; diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index e524eaabd23c..02de8073ef47 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -2688,82 +2688,86 @@ describe('parser', function() { expect($parse(':: ').literal).toBe(true); })); - it('should only become stable when all the properties of an object have defined values', inject(function($parse, $rootScope, log) { - var fn = $parse('::{foo: foo, bar: bar}'); - $rootScope.$watch(fn, function(value) { log(value); }, true); - - expect(log.empty()).toEqual([]); - expect($rootScope.$$watchers.length).toBe(1); - - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(1); - expect(log.empty()).toEqual([{foo: undefined, bar: undefined}]); - - $rootScope.foo = 'foo'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(1); - expect(log.empty()).toEqual([{foo: 'foo', bar: undefined}]); - - $rootScope.foo = 'foobar'; - $rootScope.bar = 'bar'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(0); - expect(log.empty()).toEqual([{foo: 'foobar', bar: 'bar'}]); - - $rootScope.foo = 'baz'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(0); - expect(log.empty()).toEqual([]); - })); - - it('should only become stable when all the elements of an array have defined values', inject(function($parse, $rootScope, log) { - var fn = $parse('::[foo,bar]'); - $rootScope.$watch(fn, function(value) { log(value); }, true); - - expect(log.empty()).toEqual([]); - expect($rootScope.$$watchers.length).toBe(1); - - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(1); - expect(log.empty()).toEqual([[undefined, undefined]]); - - $rootScope.foo = 'foo'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(1); - expect(log.empty()).toEqual([['foo', undefined]]); - - $rootScope.foo = 'foobar'; - $rootScope.bar = 'bar'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(0); - expect(log.empty()).toEqual([['foobar', 'bar']]); - - $rootScope.foo = 'baz'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(0); - expect(log.empty()).toEqual([]); - })); - - it('should only become stable when all the elements of an array have defined values at the end of a $digest', inject(function($parse, $rootScope, log) { - var fn = $parse('::[foo]'); - $rootScope.$watch(fn, function(value) { log(value); }, true); - $rootScope.$watch('foo', function() { if ($rootScope.foo === 'bar') {$rootScope.foo = undefined; } }); - - $rootScope.foo = 'bar'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(2); - expect(log.empty()).toEqual([['bar'], [undefined]]); - - $rootScope.foo = 'baz'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(1); - expect(log.empty()).toEqual([['baz']]); - - $rootScope.bar = 'qux'; - $rootScope.$digest(); - expect($rootScope.$$watchers.length).toBe(1); - expect(log).toEqual([]); - })); + [true, false].forEach(function(isDeep) { + describe(isDeep ? 'deepWatch' : 'watch', function() { + it('should only become stable when all the properties of an object have defined values', inject(function($parse, $rootScope, log) { + var fn = $parse('::{foo: foo, bar: bar}'); + $rootScope.$watch(fn, function(value) { log(value); }, isDeep); + + expect(log.empty()).toEqual([]); + expect($rootScope.$$watchers.length).toBe(1); + + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(1); + expect(log.empty()).toEqual([{foo: undefined, bar: undefined}]); + + $rootScope.foo = 'foo'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(1); + expect(log.empty()).toEqual([{foo: 'foo', bar: undefined}]); + + $rootScope.foo = 'foobar'; + $rootScope.bar = 'bar'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(0); + expect(log.empty()).toEqual([{foo: 'foobar', bar: 'bar'}]); + + $rootScope.foo = 'baz'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(0); + expect(log.empty()).toEqual([]); + })); + + it('should only become stable when all the elements of an array have defined values', inject(function($parse, $rootScope, log) { + var fn = $parse('::[foo,bar]'); + $rootScope.$watch(fn, function(value) { log(value); }, isDeep); + + expect(log.empty()).toEqual([]); + expect($rootScope.$$watchers.length).toBe(1); + + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(1); + expect(log.empty()).toEqual([[undefined, undefined]]); + + $rootScope.foo = 'foo'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(1); + expect(log.empty()).toEqual([['foo', undefined]]); + + $rootScope.foo = 'foobar'; + $rootScope.bar = 'bar'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(0); + expect(log.empty()).toEqual([['foobar', 'bar']]); + + $rootScope.foo = 'baz'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(0); + expect(log.empty()).toEqual([]); + })); + + it('should only become stable when all the elements of an array have defined values at the end of a $digest', inject(function($parse, $rootScope, log) { + var fn = $parse('::[foo]'); + $rootScope.$watch(fn, function(value) { log(value); }, isDeep); + $rootScope.$watch('foo', function() { if ($rootScope.foo === 'bar') {$rootScope.foo = undefined; } }); + + $rootScope.foo = 'bar'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(2); + expect(log.empty()).toEqual([['bar'], [undefined]]); + + $rootScope.foo = 'baz'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(1); + expect(log.empty()).toEqual([['baz']]); + + $rootScope.bar = 'qux'; + $rootScope.$digest(); + expect($rootScope.$$watchers.length).toBe(1); + expect(log).toEqual([]); + })); + }); + }); }); }); From 2087b25aa394e96dd8196850824da9891d7fabd3 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Wed, 29 Mar 2017 19:11:42 -0700 Subject: [PATCH 2/3] fixup! fix($parse): standardize one-time literal vs non-literal and interceptors --- src/ng/directive/ngClass.js | 6 +++--- src/ng/parse.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/ng/directive/ngClass.js b/src/ng/directive/ngClass.js index 440c0b839d2b..090c2c387f8c 100644 --- a/src/ng/directive/ngClass.js +++ b/src/ng/directive/ngClass.js @@ -91,9 +91,9 @@ function classDirective(name, selector) { } function ngClassWatchAction(newClassString) { - //When using a one-time binding the newClassString will return - //the pre-interceptor value until the one-time is complete - if (newClassString && typeof newClassString !== "string") { + // When using a one-time binding the newClassString will return + // the pre-interceptor value until the one-time is complete + if (!isString(newClassString)) { newClassString = toClassString(newClassString); } diff --git a/src/ng/parse.js b/src/ng/parse.js index 78ea77e97657..23d9840fce70 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -1852,7 +1852,7 @@ function $ParseProvider() { } function oneTimeWatchDelegate(scope, listener, objectEquality, parsedExpression, prettyPrintExpression) { - var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined; + var isDone = parsedExpression.literal ? isAllDefined : isDefined; var unwatch, lastValue; if (parsedExpression.inputs) { unwatch = inputsWatchDelegate(scope, oneTimeListener, objectEquality, parsedExpression, prettyPrintExpression); @@ -1869,9 +1869,9 @@ function $ParseProvider() { if (isFunction(listener)) { listener(value, old, scope); } - if (doneCriteria(value)) { + if (isDone(value)) { scope.$$postDigest(function() { - if (doneCriteria(lastValue)) { + if (isDone(lastValue)) { unwatch(); } }); From 2b6c11b8cf1969bd0ceb7a74bd98919a5dfd45c9 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Fri, 31 Mar 2017 09:13:35 +0200 Subject: [PATCH 3/3] update variable name --- src/ng/parse.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ng/parse.js b/src/ng/parse.js index 23d9840fce70..3f16d1330622 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -1900,7 +1900,7 @@ function $ParseProvider() { var watchDelegate = parsedExpression.$$watchDelegate; var useInputs = false; - var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined; + var isDone = parsedExpression.literal ? isAllDefined : isDefined; function regularInterceptedExpression(scope, locals, assign, inputs) { var value = useInputs && inputs ? inputs[0] : parsedExpression(scope, locals, assign, inputs); @@ -1912,7 +1912,7 @@ function $ParseProvider() { var result = interceptorFn(value, scope, locals); // we only return the interceptor's result if the // initial value is defined (for bind-once) - return doneCriteria(value) ? result : value; + return isDone(value) ? result : value; } var fn = parsedExpression.oneTime ? oneTimeInterceptedExpression : regularInterceptedExpression;