refactor($parse): don't use bind-once interceptor for non-bind-once expressions #9961

caitp · 2014-11-07T21:16:00Z

Side-effects:

Logic for allOrNothing watches now lives in $interpolate rather than $parse

caitp · 2014-11-07T21:18:35Z

It's a bit messy, so I'm open to suggestions for making it a bit smaller. @lgalfaso can you review please

lgalfaso · 2014-11-09T20:15:29Z

@caitp it does look overly complicated. What is wrong with just

diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js
index 2e24b3f..4691719 100644
--- a/src/ng/interpolate.js
+++ b/src/ng/interpolate.js
@@ -300,6 +300,7 @@ function $InterpolateProvider() {
       }

       function parseStringifyInterceptor(value) {
+        if (isUndefined(value)) return;
         try {
           return stringify(getValue(value));
         } catch (err) {
diff --git a/src/ng/parse.js b/src/ng/parse.js
index 4b0a239..47b7af0 100644
--- a/src/ng/parse.js
+++ b/src/ng/parse.js
@@ -1257,9 +1257,7 @@ function $ParseProvider() {
       var fn = function interceptedExpression(scope, locals) {
         var value = parsedExpression(scope, locals);
         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) || interceptorFn.$stateful ? result : value;
+        return result;
       };

       // Propagate $$watchDelegates other then inputsWatchDelegate

??

caitp · 2014-11-09T20:36:41Z

the problem is that it's not correct. we need to split these paths apart, because they do different things, and they screw up when you cross the streams.

lgalfaso · 2014-11-09T20:38:38Z

can you please post an example?

caitp · 2014-11-09T20:43:54Z

the example is, bind-once, all-or-nothing, and regular watches all have different behaviours, they are not always compatible (which is why we have this bug in the first place), and it's a really stupid idea to try and fit them into a box together, because they are just different.

The result of trying to make them all work using the same path, will be confusing, incorrect code. Plain and simple. It will be slightly smaller code, but it will be wrong.

Honestly, the interceptors and watch delegates thing was a recipe for bad code to begin with, but it's what we've got.

caitp · 2014-11-09T20:45:36Z

interestingly there are some test failures that I wasn't seeing locally, so I'll need to change this a bit to fix those, this isn't ready for review yet.

jbedard · 2014-11-09T22:45:46Z

src/ng/compile.js

@@ -1936,7 +1936,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                  }
                  return lastValue = parentValue;
                };
-                parentValueWatch.$stateful = true;


I'm pretty sure this needs to stay. parentValueWatch depends on isolteBindingContext which makes it stateful...

it's not, setting it as $stateful was just a really stupid hack, it's not the right way to do this. it was added because the decision was to make $stateful equate to essentially the opposite of bind-once

which is, obviously, not the right thing to do, it just doesn't make sense, and makes the spaghetti monster even bigger.

I think that is incorrect. $stateful means the output depends on more then just the input, such as parentValueWatch depending on isolateBindingContext. This is for the input-watching, nothing to do with bind-once.

this isn't necessary, should not be necessary, and the whole fact that we have a $stateful flag is unnecessary and should be deleted from the codebase.

That is why you have $compile isolate scope tests failing...

I think you're misunderstanding what $stateful is for, but ok.

I'm not misunderstanding it, what I'm understanding is that it was a bad idea to begin with. We don't need hacks to improve performance, what we need is better algorithms, and better VM implementations, to improve performance.

seriously, we have turned a once-simple parser, and turned it into an extremely complicated mess. we should not have done this, this was not the right thing to do.

we've made it harder on users, and we've made it harder on maintainers. this was not the right thing to do, even if it artificially appears faster.

The last 200 lines of parse.js involving all the $watch logic is ugly, for sure, I'm not denying that :P

we've made it harder on users, and we've made it harder on maintainers.
this was not the right thing to do, even if it artificially appears faster.

Maybe it is about time for $parse to be split into

expression parsing and evaluation

one-time binding and other enhancements

jbedard · 2014-11-09T23:24:39Z

@lgalfaso I think your suggestion would break interpolation of undefined values?

How about removing the use of the interceptorFn from $interpolate (jbedard@8452a54) and then handling one-time interceptors differently then others (jbedard@0c3ea3c)?

lgalfaso · 2014-11-10T10:21:23Z

@jbernard can you write a test that would pass before and would fail with
the patch? The reason I am asking is because all the existing tests pass,
so it would be nice to add such a test

caitp · 2014-11-10T13:53:52Z

alright, i put the $stateful back... but i'm not happy about it!

I removed that as a last minute thing, which is why i didn't see failures before submitting the CL.

I don't think we should refactor the parser to have an "evaluation" section separate from the parsing.

Ideally, we should have something like this:

parser generates an AST
in non-CSP mode, each AST node is visited and used to generate a getter and setter
in CSP mode, each get/set operation visits the entire AST with a visitor specific to getting/setting

This is probably the most straight forward, easily understood approach --- and it's also essentially what we were doing before we introduced the bind-once nonsense and the filter "optimization".

If bind-once and filter optimization code generation were moved to the AST-visiting level rather than this hack at the end of the pipeline, it would be a lot easier to understand.

Unfortunately, having multiple types of AST visitors increases complexity a lot over the original code, so it might not be doable. But if we're going to have this crap with bind-once and filter optimizations, it's the right thing to do

jbedard · 2014-11-11T05:17:17Z

@lgalfaso it doesn't break what I thought it would (because .join([undefined]) returns a blank string), but that change to parse.js breaks any other cases where an interceptorFn converts undefined to non-undefined (such as ng-bind-html): jbedard@bc0e4c0

That's why I think we need a different interceptedExpression for bind-once, like @caitp has in this PR.

However I think removing the use of interceptors in $interpolate is better then having multiple: jbedard@4bcf1b1

lgalfaso · 2014-11-11T20:00:36Z

src/ng/interpolate.js

@@ -308,6 +312,20 @@ function $InterpolateProvider() {
          $exceptionHandler(newErr);
        }
      }
+
+      function allOrNothingParseStringifyInterceptor(value) {


Why do we need two different interceptors? the allOrNothingParseStringifyInterceptor should work well in both cases

the other case will return the empty string for undefined, which breaks allOrNothing mode.

I am saying that if the input is undefined, then return undefined regardless of if this is all-or-nothing or not

that breaks interpolation, it never worked that way.

I recall jbedard had another version of this that avoided interceptors here entirely though, which is probably good, i have to look at it though

Why would it break interpolation? ['foo', undefined, 'bar'].toString() === 'foobar'

caitp · 2014-11-11T20:41:56Z

so this is a pretty tiny fixup now, and removes the weirdness from before. We can look at getting rid of the remaining weirdness later, but lets get this in today. Look good to you Lucas?

lgalfaso · 2014-11-11T21:11:57Z

agree, we can look later into doing a second cleanup. Can you check the why of the test failures?

caitp · 2014-11-11T21:14:55Z

looks like flakes

lgalfaso · 2014-11-11T21:15:50Z

LGTM

caitp · 2014-11-12T00:58:31Z

It's not a flake, getting rid of the interceptor breaks interpolation -______- very angry with this

caitp · 2014-11-12T00:58:55Z

Sorry @jbedard but that approach isn't going to work :\

caitp · 2014-11-12T01:06:16Z

gonna try plan B) ._.

@jbedard

…xpressions Side-effects: - Logic for allOrNothing watches now lives in $intercept rather than $parse Credit to @jbedard for idea to remove $watch interceptors craziness from $interpolate. Closes angular#9958

jbedard · 2014-11-12T01:41:15Z

Really? :( I had all the tests passing in my branch...
On 11 Nov 2014 17:30, "Caitlin Potter" [email protected] wrote:

Closed #9961 #9961 via b7afd11
b7afd11
.

—
Reply to this email directly or view it on GitHub
#9961 (comment).

caitp · 2014-11-12T01:44:20Z

@jbedard it was failing docsapp tests, and E2E tests, but passing unit tests. When trying the docsapp failure in the browser, the bound view was not being updated at all, it was pretty broken :(

jbedard · 2014-11-13T05:10:37Z

Was that something that can be unit tested? Could also add jbedard@bc0e4c0, and I saw someone wanting bind-once working with ng-sanitize which sounds similar...

caitp force-pushed the issue-9958 branch 2 times, most recently from 773ee2d to 83ce6aa Compare November 7, 2014 21:17

caitp added type: refactor component: $parse labels Nov 7, 2014

caitp assigned lgalfaso Nov 7, 2014

caitp added this to the 1.3.3 milestone Nov 7, 2014

mary-poppins added cla: yes and removed cla: no labels Nov 7, 2014

jbedard reviewed Nov 9, 2014
View reviewed changes

caitp force-pushed the issue-9958 branch from 83ce6aa to df8813c Compare November 10, 2014 13:48

lgalfaso reviewed Nov 11, 2014
View reviewed changes

caitp force-pushed the issue-9958 branch 4 times, most recently from 93ac49f to b062fb3 Compare November 11, 2014 20:41

caitp force-pushed the issue-9958 branch from b062fb3 to 3c74c3a Compare November 12, 2014 01:06

refactor($parse): don't use bind-once interceptor for non-bind-once e…

b06c581

…xpressions Side-effects: - Logic for allOrNothing watches now lives in $intercept rather than $parse Credit to @jbedard for idea to remove $watch interceptors craziness from $interpolate. Closes angular#9958

caitp force-pushed the issue-9958 branch from 3c74c3a to b06c581 Compare November 12, 2014 01:07

caitp closed this in b7afd11 Nov 12, 2014

jbedard mentioned this pull request Nov 19, 2014

test($interpolate): adding tests for watching $interpolate functions #10119

Closed

refactor($parse): don't use bind-once interceptor for non-bind-once expressions #9961

refactor($parse): don't use bind-once interceptor for non-bind-once expressions #9961

Conversation

caitp commented Nov 7, 2014

caitp commented Nov 7, 2014

lgalfaso commented Nov 9, 2014

caitp commented Nov 9, 2014

lgalfaso commented Nov 9, 2014

caitp commented Nov 9, 2014

caitp commented Nov 9, 2014

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 Nov 9, 2014

lgalfaso commented Nov 10, 2014

caitp commented Nov 10, 2014

jbedard commented Nov 11, 2014

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

caitp commented Nov 11, 2014

lgalfaso commented Nov 11, 2014

caitp commented Nov 11, 2014

lgalfaso commented Nov 11, 2014

caitp commented Nov 12, 2014

caitp commented Nov 12, 2014

caitp commented Nov 12, 2014

jbedard commented Nov 12, 2014

caitp commented Nov 12, 2014

jbedard commented Nov 13, 2014