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

jbedard · 2017-05-27T07:40:42Z

This basically applies the same as b5118ac to interceptors.

For example normally when watching [var] the var only needs to be shallow-watched. But if that expression then gets wrapped in an interceptor such as $parse('[var]', interceptor) we must assume the interceptor is non-pure and might read state from within the var (just like filters/functions...).

This tries to preserve shallow watching of things behind operators such as ! by distinguishing if an expression is pure due to an operator such as ! ("absolute") or pure only if no parent operation wants to deep watch it ("relative"). This way when wrapped in an interceptor the "absolute" ones can still be shallow watched, while the "relative" ones that the interceptor has access to can no longer be shallow watched.

This still doesn't fully restore the 1.6.3 ng-class functionality because it still doesn't do a deep-watch for one-time bindings like 1.6.3 did. The one (famous last words...) remaining regression from 1.6.3 is demonstrated by the modified ng-class spec (objects-in-literals-with-interceptors-in-one-time bindings). This is due to this annoying condition which I really wish I could find a better method of doing...

jbedard · 2017-05-27T07:41:09Z

src/ng/parse.js

@@ -622,6 +622,9 @@ function isStateless($filter, filterName) {
  return !fn.$stateful;
 }

+var PURITY_ABSOLUTE = 1;


Better terminology for these...?

jbedard · 2017-05-27T07:47:03Z

src/ng/parse.js

+              // Remove the isPure flag of inputs when it is not absolute because they are now wrapped in a
+              // potentially non-pure interceptor function.
+              if (e.isPure === PURITY_RELATIVE) {
+                return function depurifier(s) { return e(s); };


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.

where is depurifier defined?

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

jbedard · 2017-05-31T04:32:18Z

test/ng/directive/ngClassSpec.js

@@ -629,7 +645,7 @@ describe('ngClass', function() {
    }));

    it('should not be copied when using one-time binding', inject(function($compile, $rootScope) {
-      element = $compile('<div ng-class="::{foo: veryLargeObj, bar: bar}"></div>')($rootScope);
+      element = $compile('<div ng-class="::{foo: !!veryLargeObj, bar: bar}"></div>')($rootScope);


This is the change I'd like to avoid in 1.6.x by reverting the ng-class half of 189461f in 1.6

jbedard · 2017-06-05T20:11:55Z

@lgalfaso would be great to get a review from you if you have a chance

lgalfaso

I know that this PR works as it solves the specific issue, but I think it does so at a very high cost. What is currently at $parse are two things

The lexer, tokenizer, expression generation and evaluation (this includes the two phase mechanism to only evaluate things once even when there is a change)
One-time binding, unbinding on literals and all the other interceptor functionality, including watch delegates in its many forms

So far, the contract between these two parts was minimal, in theory it was possible to split these two into two different parts. This PR makes this theoretical separation hard. What would it take to get this separation back?

lgalfaso · 2017-06-05T20:40:44Z

src/ng/parse.js

+              // Remove the isPure flag of inputs when it is not absolute because they are now wrapped in a
+              // potentially non-pure interceptor function.
+              if (e.isPure === PURITY_RELATIVE) {
+                return function depurifier(s) { return e(s); };


where is depurifier defined?

jbedard · 2017-06-05T21:02:47Z

I agree with keeping those two parts separate. In fact I've often been tempted to move part2 into rootScope.js because that stuff is really about how to watch expression, not about the expressions themselves. Basically parse.js would parse strings and output functions+metadata. That metadata would be today's constant, literal and inputs flags. Then rootScope may or may not use those properties to watch those functions. Then you could even declare your own functions and add these properties, this would replace things like the interpolate watch delegate with .constant = true.

However I don't think this PR really makes it worse if you consider addInterceptor to be part of part1. Interceptors are part of the $parse method signature so I would consider them part1, not part2.

See #16015 which I think moves this direction a bit by separating $$watchDelegates out of addInterceptor.

lgalfaso · 2017-06-05T21:15:51Z

src/ng/parse.js

-        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.


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.

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

I've updated the comment above this to remove the (incorrect) use of "pure function" terminology.

lgalfaso · 2017-06-05T22:02:45Z

With the clarifications and the changes in the comments, LGTM.

…or fn Fixes angular#15905 Closes angular#16018

googlebot added the cla: yes label May 27, 2017

jbedard commented May 27, 2017

View reviewed changes

jbedard force-pushed the 15905-parse-intercepted-purity branch from 5c3f3ff to 6699f60 Compare May 27, 2017 20:48

jbedard commented May 31, 2017

View reviewed changes

jbedard mentioned this pull request May 31, 2017

Revert "fix($parse): standardize one-time literal vs non-literal and interceptors" #15958

Merged

jbedard force-pushed the 15905-parse-intercepted-purity branch from 6699f60 to 8edbf5c Compare June 2, 2017 04:22

jbedard added component: $parse severity: broken expected use type: bug labels Jun 2, 2017

jbedard added this to the 1.6.5 milestone Jun 2, 2017

lgalfaso reviewed Jun 5, 2017

View reviewed changes

jbedard force-pushed the 15905-parse-intercepted-purity branch 3 times, most recently from a90bc21 to 6c8933d Compare June 13, 2017 03:14

fix($parse): do not shallow-watch inputs when wrapped in an intercept…

869644e

…or fn Fixes angular#15905 Closes angular#16018

jbedard force-pushed the 15905-parse-intercepted-purity branch from 6c8933d to 869644e Compare June 13, 2017 03:52

jbedard merged commit b12a0b7 into angular:master Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jbedard commented May 27, 2017

jbedard May 27, 2017

jbedard May 27, 2017

lgalfaso Jun 5, 2017

jbedard Jun 5, 2017

jbedard May 31, 2017

jbedard commented Jun 5, 2017

lgalfaso left a comment

lgalfaso Jun 5, 2017

jbedard commented Jun 5, 2017

lgalfaso Jun 5, 2017

jbedard Jun 5, 2017

jbedard Jun 9, 2017

lgalfaso commented Jun 5, 2017

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

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

Conversation

jbedard commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jun 5, 2017

lgalfaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgalfaso commented Jun 5, 2017