diff --git a/src/ng/parse.js b/src/ng/parse.js index c9fa42b63..4455129d7 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -622,15 +622,44 @@ function isStateless($filter, filterName) { return !fn.$stateful; } -function findConstantAndWatchExpressions(ast, $filter) { +// Detect nodes which could depend on non-shallow state of objects +function isPure(node, parentIsPure) { + switch (node.type) { + // Computed members might invoke a stateful toString() + case AST.MemberExpression: + if (node.computed) { + return false; + } + break; + + // Unary always convert to primative + case AST.UnaryExpression: + return true; + + // The binary + operator can invoke a stateful toString(). + case AST.BinaryExpression: + return node.operator !== '+'; + + // Functions / filters probably read state from within objects + case AST.CallExpression: + return false; + } + + return (undefined === parentIsPure) || parentIsPure; +} + +function findConstantAndWatchExpressions(ast, $filter, parentIsPure) { var allConstants; var argsToWatch; var isStatelessFilter; + + var astIsPure = ast.isPure = isPure(ast, parentIsPure); + switch (ast.type) { case AST.Program: allConstants = true; forEach(ast.body, function(expr) { - findConstantAndWatchExpressions(expr.expression, $filter); + findConstantAndWatchExpressions(expr.expression, $filter, astIsPure); allConstants = allConstants && expr.expression.constant; }); ast.constant = allConstants; @@ -640,26 +669,26 @@ function findConstantAndWatchExpressions(ast, $filter) { ast.toWatch = []; break; case AST.UnaryExpression: - findConstantAndWatchExpressions(ast.argument, $filter); + findConstantAndWatchExpressions(ast.argument, $filter, astIsPure); ast.constant = ast.argument.constant; ast.toWatch = ast.argument.toWatch; break; case AST.BinaryExpression: - findConstantAndWatchExpressions(ast.left, $filter); - findConstantAndWatchExpressions(ast.right, $filter); + findConstantAndWatchExpressions(ast.left, $filter, astIsPure); + findConstantAndWatchExpressions(ast.right, $filter, astIsPure); ast.constant = ast.left.constant && ast.right.constant; ast.toWatch = ast.left.toWatch.concat(ast.right.toWatch); break; case AST.LogicalExpression: - findConstantAndWatchExpressions(ast.left, $filter); - findConstantAndWatchExpressions(ast.right, $filter); + findConstantAndWatchExpressions(ast.left, $filter, astIsPure); + findConstantAndWatchExpressions(ast.right, $filter, astIsPure); ast.constant = ast.left.constant && ast.right.constant; ast.toWatch = ast.constant ? [] : [ast]; break; case AST.ConditionalExpression: - findConstantAndWatchExpressions(ast.test, $filter); - findConstantAndWatchExpressions(ast.alternate, $filter); - findConstantAndWatchExpressions(ast.consequent, $filter); + findConstantAndWatchExpressions(ast.test, $filter, astIsPure); + findConstantAndWatchExpressions(ast.alternate, $filter, astIsPure); + findConstantAndWatchExpressions(ast.consequent, $filter, astIsPure); ast.constant = ast.test.constant && ast.alternate.constant && ast.consequent.constant; ast.toWatch = ast.constant ? [] : [ast]; break; @@ -668,9 +697,9 @@ function findConstantAndWatchExpressions(ast, $filter) { ast.toWatch = [ast]; break; case AST.MemberExpression: - findConstantAndWatchExpressions(ast.object, $filter); + findConstantAndWatchExpressions(ast.object, $filter, astIsPure); if (ast.computed) { - findConstantAndWatchExpressions(ast.property, $filter); + findConstantAndWatchExpressions(ast.property, $filter, astIsPure); } ast.constant = ast.object.constant && (!ast.computed || ast.property.constant); ast.toWatch = [ast]; @@ -680,7 +709,7 @@ function findConstantAndWatchExpressions(ast, $filter) { allConstants = isStatelessFilter; argsToWatch = []; forEach(ast.arguments, function(expr) { - findConstantAndWatchExpressions(expr, $filter); + findConstantAndWatchExpressions(expr, $filter, astIsPure); allConstants = allConstants && expr.constant; if (!expr.constant) { argsToWatch.push.apply(argsToWatch, expr.toWatch); @@ -690,8 +719,8 @@ function findConstantAndWatchExpressions(ast, $filter) { ast.toWatch = isStatelessFilter ? argsToWatch : [ast]; break; case AST.AssignmentExpression: - findConstantAndWatchExpressions(ast.left, $filter); - findConstantAndWatchExpressions(ast.right, $filter); + findConstantAndWatchExpressions(ast.left, $filter, astIsPure); + findConstantAndWatchExpressions(ast.right, $filter, astIsPure); ast.constant = ast.left.constant && ast.right.constant; ast.toWatch = [ast]; break; @@ -699,7 +728,7 @@ function findConstantAndWatchExpressions(ast, $filter) { allConstants = true; argsToWatch = []; forEach(ast.elements, function(expr) { - findConstantAndWatchExpressions(expr, $filter); + findConstantAndWatchExpressions(expr, $filter, astIsPure); allConstants = allConstants && expr.constant; if (!expr.constant) { argsToWatch.push.apply(argsToWatch, expr.toWatch); @@ -712,13 +741,13 @@ function findConstantAndWatchExpressions(ast, $filter) { allConstants = true; argsToWatch = []; forEach(ast.properties, function(property) { - findConstantAndWatchExpressions(property.value, $filter); + findConstantAndWatchExpressions(property.value, $filter, astIsPure); allConstants = allConstants && property.value.constant && !property.computed; if (!property.value.constant) { argsToWatch.push.apply(argsToWatch, property.value.toWatch); } if (property.computed) { - findConstantAndWatchExpressions(property.key, $filter); + findConstantAndWatchExpressions(property.key, $filter, astIsPure); if (!property.key.constant) { argsToWatch.push.apply(argsToWatch, property.key.toWatch); } @@ -803,7 +832,7 @@ ASTCompiler.prototype = { var intoId = self.nextId(); self.recurse(watch, intoId); self.return_(intoId); - self.state.inputs.push(fnKey); + self.state.inputs.push({name: fnKey, isPure: watch.isPure}); watch.watchId = key; }); this.state.computing = 'fn'; @@ -839,13 +868,16 @@ ASTCompiler.prototype = { watchFns: function() { var result = []; - var fns = this.state.inputs; + var inputs = this.state.inputs; var self = this; - forEach(fns, function(name) { - result.push('var ' + name + '=' + self.generateFunction(name, 's')); + forEach(inputs, function(input) { + result.push('var ' + input.name + '=' + self.generateFunction(input.name, 's')); + if (input.isPure) { + result.push(input.name, '.isPure=true;'); + } }); - if (fns.length) { - result.push('fn.inputs=[' + fns.join(',') + '];'); + if (inputs.length) { + result.push('fn.inputs=[' + inputs.map(function(i) { return i.name; }).join(',') + '];'); } return result.join(''); }, @@ -1251,6 +1283,7 @@ ASTInterpreter.prototype = { inputs = []; forEach(toWatch, function(watch, key) { var input = self.recurse(watch); + input.isPure = watch.isPure; watch.input = input; inputs.push(input); watch.watchId = key; @@ -1817,7 +1850,7 @@ function $ParseProvider() { inputExpressions = inputExpressions[0]; return scope.$watch(function expressionInputWatch(scope) { var newInputValue = inputExpressions(scope); - if (!expressionInputDirtyCheck(newInputValue, oldInputValueOf, parsedExpression.literal)) { + if (!expressionInputDirtyCheck(newInputValue, oldInputValueOf, inputExpressions.isPure)) { lastResult = parsedExpression(scope, undefined, undefined, [newInputValue]); oldInputValueOf = newInputValue && getValueOf(newInputValue); } @@ -1837,7 +1870,7 @@ function $ParseProvider() { for (var i = 0, ii = inputExpressions.length; i < ii; i++) { var newInputValue = inputExpressions[i](scope); - if (changed || (changed = !expressionInputDirtyCheck(newInputValue, oldInputValueOfValues[i], parsedExpression.literal))) { + if (changed || (changed = !expressionInputDirtyCheck(newInputValue, oldInputValueOfValues[i], inputExpressions[i].isPure))) { oldInputValues[i] = newInputValue; oldInputValueOfValues[i] = newInputValue && getValueOf(newInputValue); } diff --git a/test/ng/directive/ngClassSpec.js b/test/ng/directive/ngClassSpec.js index ae671d050..655f3b9ce 100644 --- a/test/ng/directive/ngClassSpec.js +++ b/test/ng/directive/ngClassSpec.js @@ -567,6 +567,26 @@ describe('ngClass', function() { }) ); + //https://github.com/angular/angular.js/issues/15960#issuecomment-299109412 + it('should always reevaluate filters with non-primitive inputs within literals', function() { + module(function($filterProvider) { + $filterProvider.register('foo', valueFn(function(o) { + return o.a || o.b; + })); + }); + + inject(function($rootScope, $compile) { + $rootScope.testObj = {}; + element = $compile('
')($rootScope); + + $rootScope.$apply(); + expect(element).not.toHaveClass('x'); + + $rootScope.$apply('testObj.a = true'); + expect(element).toHaveClass('x'); + }); + }); + describe('large objects', function() { var getProp; var veryLargeObj; diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index 13d7a1003..515eed29e 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -2973,6 +2973,244 @@ describe('parser', function() { expect(watcherCalls).toBe(1); })); + it('should not reevaluate filters in literals with non-primitive input that does support valueOf()', + inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.date = new Date(1234567890123); + + var watcherCalls = 0; + scope.$watch('[(date | foo)]', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + })); + + it('should reevaluate filters in literals with non-primitive input that does support valueOf() when' + + ' valueOf() value changes', + inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.date = new Date(1234567890123); + + var watcherCalls = 0; + scope.$watch('[(date | foo)]', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.date.setTime(1234567890); + + scope.$digest(); + expect(filterCalls).toBe(2); + expect(watcherCalls).toBe(2); + })); + + it('should not reevaluate literals containing filters with non-primitive input that does support valueOf()' + + ' when the instance changes but valueOf() does not', inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.date = new Date(1234567890123); + + var watcherCalls = 0; + scope.$watch($parse('[(date | foo)]'), function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(watcherCalls).toBe(1); + expect(filterCalls).toBe(1); + + scope.date = new Date(1234567890123); + scope.$digest(); + expect(watcherCalls).toBe(1); + expect(filterCalls).toBe(1); + })); + + it('should not reevaluate filters with literal input containing primitives', inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + var watcherCalls = 0; + scope.$watch('[a] | foo', function(input) { + watcherCalls++; + }); + + scope.$apply('a = 1'); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$apply('a = 2'); + expect(filterCalls).toBe(2); + expect(watcherCalls).toBe(2); + })); + + it('should not reevaluate filters within literals with primitive inputs', inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.prim = 1234567890123; + + var watcherCalls = 0; + scope.$watch('[(prim | foo)]', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + })); + + it('should always reevaluate filters with non-primitive inputs within literals', + inject(function($parse) { + $filterProvider.register('foo', valueFn(function(input) { + return input.b > 0; + })); + + scope.$watch('[(a | foo)]', function() {}); + + // Would be great if filter-output was checked for changes and this didn't throw... + expect(function() { scope.$apply('a = {b: 1}'); }).toThrowMinErr('$rootScope', 'infdig'); + })); + + it('should always reevaluate filters with literal input containing non-primitives', + inject(function($parse) { + scope.$watch('[a] | filter', function() {}); + + scope.$apply('a = 1'); + + // Would be great if filter-output was checked for changes and this didn't throw... + expect(function() { scope.$apply('a = {}'); }).toThrowMinErr('$rootScope', 'infdig'); + })); + + it('should not reevaluate filters with non-primitive input that gets simplified via unary operators', + inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.obj = {}; + + var watcherCalls = 0; + scope.$watch('!obj | foo:!obj', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + })); + + it('should not reevaluate filters with non-primitive input that gets simplified via non plus/concat binary operators', + inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.obj = {}; + + var watcherCalls = 0; + scope.$watch('1 - obj | foo:(1 * obj)', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + })); + + it('should reevaluate filters with non-primitive input that gets simplified via plus/concat', inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + scope.obj = {}; + + var watcherCalls = 0; + scope.$watch('1 + obj | foo', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(2); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(3); + expect(watcherCalls).toBe(1); + })); + + it('should reevaluate computed member expressions with non-primitive input', inject(function($parse) { + var toStringCalls = 0; + + scope.obj = {}; + scope.key = { + toString: function() { + toStringCalls++; + return 'foo'; + } + }; + + var watcherCalls = 0; + scope.$watch('obj[key]', function(input) { + watcherCalls++; + }); + + scope.$digest(); + expect(toStringCalls).toBe(2); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(toStringCalls).toBe(3); + expect(watcherCalls).toBe(1); + })); + it('should always reevaluate filters with non-primitive input created with null prototype', inject(function($parse) { var filterCalls = 0; @@ -3027,7 +3265,7 @@ describe('parser', function() { })); it('should reevaluate filters with non-primitive input that does support valueOf() when' + - 'valueOf() value changes', inject(function($parse) { + ' valueOf() value changes', inject(function($parse) { var filterCalls = 0; $filterProvider.register('foo', valueFn(function(input) { filterCalls++; @@ -3055,7 +3293,7 @@ describe('parser', function() { expect(watcherCalls).toBe(1); })); - it('should invoke interceptorFns if they are flagged as having $stateful', + it('should always invoke interceptorFns if they are flagged as having $stateful', inject(function($parse) { var called = false; function interceptor() { @@ -3078,6 +3316,23 @@ describe('parser', function() { expect(called).toBe(true); })); + it('should not reevaluate literals with non-primitive input', inject(function($parse) { + var obj = scope.obj = {}; + + var parsed = $parse('[obj]'); + var watcherCalls = 0; + scope.$watch(parsed, function(input) { + expect(input[0]).toBe(obj); + watcherCalls++; + }); + + scope.$digest(); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(watcherCalls).toBe(1); + })); + it('should not reevaluate literals with non-primitive input that does support valueOf()', inject(function($parse) { @@ -3097,6 +3352,25 @@ describe('parser', function() { expect(watcherCalls).toBe(1); })); + it('should reevaluate literals with non-primitive input when valueOf() changes', inject(function($parse) { + var date = scope.date = new Date(); + + var parsed = $parse('[date]'); + var watcherCalls = 0; + scope.$watch(parsed, function(input) { + expect(input[0]).toBe(date); + watcherCalls++; + }); + + scope.$digest(); + expect(watcherCalls).toBe(1); + + date.setYear(1901); + + scope.$digest(); + expect(watcherCalls).toBe(2); + })); + it('should not reevaluate literals with non-primitive input that does support valueOf()' + ' when the instance changes but valueOf() does not', inject(function($parse) {