fix($parse): respect the interceptor.$stateful flag #16015

jbedard · 2017-05-26T06:49:09Z

~~This actually depends on #16009 because that (unnecessary) $stateful flag was previously being ignored in the case of the failing test.~~

jbedard · 2017-06-02T04:33:44Z

For the failing test (which depends on ~~#16009~~ the second commit): previously it only passed because the inputsWatchDelegate was incorrectly used due to this block assigning the inputs then the inputsWatchDelegate was later used because of those inputs. This is incorrect because a $stateful filter/interceptor must always be invoked, not only if it's inputs change, like the added tests demonstrate.

gkalpak · 2017-06-02T12:44:19Z

src/ng/parse.js


-      // Propagate or create inputs / $$watchDelegates
-      useInputs = !parsedExpression.inputs;
-      if (watchDelegate && watchDelegate !== inputsWatchDelegate) {


If we remove this, what happens with "custom" $$watchDelegates (such as the ones added by $interpolate)?

This actually fixes a bug because that lost $$watchDelegate knows nothing about the interceptor and is built specifically for that interpolate fn.

For example watching $parse($interpolate("{{foo}}"), function() { return "bar"; }) will always output the "foo" var even though it is wrapped in an interceptor that always returns "bar", yet executing that parsed-expression will always return "bar"...

Where is the test for this fix then? 😛

FWIW, I am still confused about what happens to $$watchDelegates when combined with interceptors? Are they just ignored?

Yeah they are just ignored, because the interceptor might have made that $$watchDelegate completely invalid. However the returned ("interecepted"?) fn might have some constant/oneTime/inputs properties that might cause a $$watchDelegate to be added.

Here's an example, I'll try to add a test for this...

$parse($interpolate("a{{b}}"), function(x) { return x + 'c'; })

$interpolate adds a special $$watchDelegate for an efficient way to watch the interpolated-expression, but that delegate of course knows nothing about the x + 'c' interceptor. Previously this line we're commenting on would blindly copy the delegate creating an expression which produces the correct value when executed (with the 'c' concatenated at the end) but the wrong value when watched due to the use of a $$watchDelegate that knows nothing about the x + 'c'...

I've added a test for this: "should not propogate $$watchDelegate to the interceptor wrapped expression"

I see. And we are fine with that because we never use $$watchDelegate with interceptors in core, right?
I might add a comment (where?) about that, i.e. the you can't use $$watchDelegate with interceptors.

Narretz · 2017-06-12T12:47:41Z

Looks like a few tests failed. And there's also an eslint error

jbedard · 2017-06-12T16:11:44Z

Fixed the lint issue. Not sure what the test errors were (don't think they failed before), we'll see if they fail again...

jbedard · 2017-06-16T08:46:13Z

I think the remaining test failure is actually caused by #16021. I'll probably end up trying to fix both in this PR since they seem overlap a lot...

jbedard · 2017-07-10T19:19:02Z

@lgalfaso would be great to get your review/opinion if you have a chance

lgalfaso

Overall, I think it is a good change and a nice cleanup. Just some minor comments.

lgalfaso · 2017-07-10T20:32:16Z

src/ng/parse.js

+
+    function chainInterceptors(first, second) {
+      function chainedInterceptor(value, scope, locals) {
+        return second(first(value, scope, locals), scope, locals);


Are you sure that locals should be sent to both interceptors?

Good question. I'll look into it and try to add some tests...

This is equivalent to today. The interceptor is passed the scope and locals. Then if it is double-intercepted then it will pass them through until reaching the original expression.

I would be tempted to remove these args from the interceptor though, and only pass them to the original wrapped expression. Probably in a separate PR though...

See #16091 which might be nice to merge before this PR.

This method is now simplified after #16091 merged.

lgalfaso · 2017-07-10T20:32:54Z

src/ng/parse.js

+      // Extract any existing interceptors out of the parsedExpression
+      // to ensure the original parsedExpression is always the $$intercepted
+      if (parsedExpression.$$interceptor) {
+        interceptorFn = chainInterceptors(parsedExpression.$$interceptor, interceptorFn);


I understand why this chaining is needed, just wonder if there is a cleaner solution.

This is all internal so it can always be changed later. I'm open to any ideas though 👍

gkalpak · 2017-07-11T06:57:03Z

src/ng/parse.js

+      var exp = parsedExpression.$$intercepted || parsedExpression;
+      var post = parsedExpression.$$interceptor || identity;
+
+      var useInputs = parsedExpression.inputs && !exp.inputs;


How could parsedExpresion.inputs be truthy and parsedException.$$intercepted.inputs be falsy?
Can you give an example scenario?

It's basically detecting this case. This is when the intercepted expression had no inputs, so the expression itself becomes the input.

For example

$parse("x", stringify): x has no inputs, but we don't want to execute stringify(x) per-digest so we set x itself as the input and useInputs = true to handle this

$parse("x | number", stringify): x | number already has x as an input so we just continue using that input (and useInputs = false)

The fact that this line basically "detects" the useInputs = true from addInterceptor is something I don't like about this PR atm. It already existed before, but I guess now it is spread across two methods which seems worse to me. But it's all internal and could always be changed...

I can't say I can fully wrap my head around this, but the tests pass, so... 😃

Which part? Anything I can explain more/better? There's a couple weird things being changed in this PR, and I also get confused after not looking at it for a while ^_^

The useInputs bit still confuses me (but in tests we trust).
_{(At some point I need to grab my debugger and go through some usecases, but this doesn't have to hold this PR back.)}

The useInputs is also the part I don't like.

Note that I think it should really be renamed to something such as theInputIsTheInterceptedExpression (where normally the input is the input to the intercepted expression).

It is also just a performance thing, dropping all useInputs == true logic would still function correctly.

Fixes angular#16021

googlebot added the cla: yes label May 26, 2017

jbedard force-pushed the parse-interceptor-stateful branch from 712ece3 to 9e8ba79 Compare May 26, 2017 08:14

Narretz mentioned this pull request May 31, 2017

perf($rootScope): do not mark $watchCollectionInterceptor as $stateful #16009

Merged

gkalpak reviewed Jun 2, 2017

View reviewed changes

jbedard mentioned this pull request Jun 5, 2017

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

Merged

jbedard force-pushed the parse-interceptor-stateful branch from 9e8ba79 to d946866 Compare June 11, 2017 22:45

jbedard force-pushed the parse-interceptor-stateful branch from d946866 to c2941b9 Compare June 12, 2017 16:09

jbedard added this to the 1.7.0 milestone Jun 12, 2017

jbedard force-pushed the parse-interceptor-stateful branch from c2941b9 to 798d277 Compare June 16, 2017 04:33

jbedard force-pushed the parse-interceptor-stateful branch 2 times, most recently from 48ede5f to 33701f1 Compare June 16, 2017 15:32

This was referenced Jun 16, 2017

fix($parse): do not shallow-watch inputs to one-time intercepted expressions #16059

Merged

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

Merged

jbedard force-pushed the parse-interceptor-stateful branch from d57f8a4 to 7fe7421 Compare June 17, 2017 17:35

jbedard force-pushed the parse-interceptor-stateful branch from 7fe7421 to 730dc11 Compare June 28, 2017 03:02

jbedard force-pushed the parse-interceptor-stateful branch 2 times, most recently from 7730530 to 139bbe0 Compare July 8, 2017 08:29

lgalfaso reviewed Jul 10, 2017

View reviewed changes

gkalpak reviewed Jul 11, 2017

View reviewed changes

gkalpak approved these changes Jul 11, 2017

View reviewed changes

jbedard added 3 commits July 14, 2017 01:54

fix($parse): respect the interceptor.$stateful flag

bab9fea

fix($parse): always pass the intercepted value to watchers

4157505

Fixes angular#16021

test(ngClass): add test for one-time objects in array literal

8654503

jbedard force-pushed the parse-interceptor-stateful branch from 139bbe0 to 8654503 Compare July 14, 2017 09:10

jbedard merged commit 2fb2d09 into angular:master Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($parse): respect the interceptor.$stateful flag #16015

fix($parse): respect the interceptor.$stateful flag #16015

jbedard commented May 26, 2017 •

edited

Loading

jbedard commented Jun 2, 2017 •

edited

Loading

gkalpak Jun 2, 2017

jbedard Jun 3, 2017

gkalpak Jun 6, 2017

gkalpak Jun 6, 2017 •

edited

Loading

jbedard Jun 9, 2017

jbedard Jun 10, 2017

jbedard Jun 11, 2017

gkalpak Jun 12, 2017

Narretz commented Jun 12, 2017

jbedard commented Jun 12, 2017

jbedard commented Jun 16, 2017

jbedard commented Jul 10, 2017

lgalfaso left a comment

lgalfaso Jul 10, 2017

jbedard Jul 10, 2017

jbedard Jul 11, 2017

jbedard Jul 11, 2017

jbedard Jul 14, 2017

lgalfaso Jul 10, 2017

jbedard Jul 11, 2017

gkalpak Jul 11, 2017

jbedard Jul 11, 2017

jbedard Jul 11, 2017

gkalpak Jul 11, 2017

jbedard Jul 11, 2017

gkalpak Jul 11, 2017

jbedard Jul 11, 2017

fix($parse): respect the interceptor.$stateful flag #16015

fix($parse): respect the interceptor.$stateful flag #16015

Conversation

jbedard commented May 26, 2017 • edited Loading

jbedard commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jun 6, 2017 • edited Loading

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

Narretz commented Jun 12, 2017

jbedard commented Jun 12, 2017

jbedard commented Jun 16, 2017

jbedard commented Jul 10, 2017

lgalfaso left a 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

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

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

Choose a reason for hiding this comment

jbedard commented May 26, 2017 •

edited

Loading

jbedard commented Jun 2, 2017 •

edited

Loading

gkalpak Jun 6, 2017 •

edited

Loading