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

fix($parse): do not shallow-watch inputs when wrapped in an interceptor fn #16018

Merged
merged 1 commit into from
Jun 13, 2017
Merged

fix($parse): do not shallow-watch inputs when wrapped in an interceptor fn #16018

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
23 changes: 16 additions & 7 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,9 @@ function isStateless($filter, filterName) {
return !fn.$stateful;
}

var PURITY_ABSOLUTE = 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Better terminology for these...?

var PURITY_RELATIVE = 2;

// Detect nodes which could depend on non-shallow state of objects
function isPure(node, parentIsPure) {
switch (node.type) {
Expand All @@ -634,18 +637,18 @@ function isPure(node, parentIsPure) {

// Unary always convert to primative
case AST.UnaryExpression:
return true;
return PURITY_ABSOLUTE;

// The binary + operator can invoke a stateful toString().
case AST.BinaryExpression:
return node.operator !== '+';
return node.operator !== '+' ? PURITY_ABSOLUTE : false;

// Functions / filters probably read state from within objects
case AST.CallExpression:
return false;
}

return (undefined === parentIsPure) || parentIsPure;
return (undefined === parentIsPure) ? PURITY_RELATIVE : parentIsPure;
}

function findConstantAndWatchExpressions(ast, $filter, parentIsPure) {
Expand Down Expand Up @@ -873,7 +876,7 @@ ASTCompiler.prototype = {
forEach(inputs, function(input) {
result.push('var ' + input.name + '=' + self.generateFunction(input.name, 's'));
if (input.isPure) {
result.push(input.name, '.isPure=true;');
result.push(input.name, '.isPure=' + JSON.stringify(input.isPure) + ';');
}
});
if (inputs.length) {
Expand Down Expand Up @@ -1960,10 +1963,16 @@ function $ParseProvider() {
fn.$$watchDelegate = watchDelegate;
fn.inputs = parsedExpression.inputs;
} else if (!interceptorFn.$stateful) {
// If there is an interceptor, but no watchDelegate then treat the interceptor like
// we treat filters - it is assumed to be a pure function unless flagged with $stateful
// Treat interceptor like filters - assume non-stateful by default and use the inputsWatchDelegate
fn.$$watchDelegate = inputsWatchDelegate;
fn.inputs = parsedExpression.inputs ? parsedExpression.inputs : [parsedExpression];
fn.inputs = (parsedExpression.inputs ? parsedExpression.inputs : [parsedExpression]).map(function(e) {
// Remove the isPure flag of inputs when it is not absolute because they are now wrapped in a
// potentially non-pure interceptor function.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand this comment and how this matches with the previous comment. Here it is stated that the interceptor can be non-pure, but before it is assumed that the interceptor is pure.

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 think the comment a few lines up is using the wrong terminology. I think it should just say "it is assumed to be non-$stateful". I'm not 100% sure if I'm using the "pure" terminology correct here, but I'll try to update the comments similar to #16015 which I think clears this up more. See: https://github.com/angular/angular.js/pull/16015/files#diff-780de070ede30180f6aedb6c5f7d57caR1964

To (try) summarizing:

  • interceptors/filters can be $stateful in which case they must always be invoked to check for changes
  • inputs can be pure in which case they only need to be shallow watched
  • inputs passed into interceptors/filters are NOT pure because the interceptor/filter might inspect the content of them
  • inputs that are pure in $parse(exp) might not be when doing $parse(exp, interceptor), but they share the same inputs array which is what this ugly .map is for

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 updated the comment above this to remove the (incorrect) use of "pure function" terminology.

if (e.isPure === PURITY_RELATIVE) {
return function depurifier(s) { return e(s); };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Creating these extra closures and executing them per-digest when watching things is a bit of a concern. But any alternative I've thought of would involve modifying https://github.com/jbedard/angular.js/blob/5c3f3ffb7478eadc72c4f90b6a1964557e355538/src/ng/parse.js#L1856 which would effect everything, not only interceptor inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

where is depurifier defined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's just the name of this wrapper function (which removes the isPure flag).

}
return e;
});
}

return fn;
Expand Down
16 changes: 16 additions & 0 deletions test/ng/directive/ngClassSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,22 @@ describe('ngClass', function() {
})
);

// https://github.com/angular/angular.js/issues/15905
it('should support a mixed literal-array/object variable', inject(function($rootScope, $compile) {
element = $compile('<div ng-class="[classVar]"></div>')($rootScope);

$rootScope.classVar = {orange: true};
$rootScope.$digest();
expect(element).toHaveClass('orange');

$rootScope.classVar.orange = false;
$rootScope.$digest();

expect(element).not.toHaveClass('orange');
})
);


it('should do value stabilization as expected when one-time binding',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::className"></div>')($rootScope);
Expand Down
19 changes: 19 additions & 0 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3277,6 +3277,25 @@ describe('parser', function() {
expect(called).toBe(true);
}));

it('should always be invoked if inputs are non-primitive', inject(function($parse) {
var called = false;
function interceptor(v) {
called = true;
return v.sub;
}

scope.$watch($parse('[o]', interceptor));
scope.o = {sub: 1};

called = false;
scope.$digest();
expect(called).toBe(true);

called = false;
scope.$digest();
expect(called).toBe(true);
}));

it('should not be invoked unless the input.valueOf() changes even if the instance changes', inject(function($parse) {
var called = false;
function interceptor(v) {
Expand Down