From 2a02678533971b296bd12f2c750ec1c52727824a Mon Sep 17 00:00:00 2001
From: herr kaste <herr.kaste@googlemail.com>
Date: Wed, 30 Mar 2016 20:48:34 +0200
Subject: [PATCH] Remove custom effects from within another effect without
 breaking.

When an effect loop is running, we cannot simply modify (mutate!) the effects
array without breaking the for-loop. So we defer the mutating to a microtask.
---
 src/lib/bind/accessors.html     |  4 +++-
 src/standard/effectBuilder.html | 31 +++++++++++++++++--------
 src/standard/notify-path.html   | 10 ++++----
 test/unit/bind.html             | 41 +++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/src/lib/bind/accessors.html b/src/lib/bind/accessors.html
index 2290344114..9419a9c6cc 100644
--- a/src/lib/bind/accessors.html
+++ b/src/lib/bind/accessors.html
@@ -79,7 +79,9 @@
 
       _effectEffects: function(property, value, effects, old, fromAbove) {
         for (var i=0, l=effects.length, fx; (i<l) && (fx=effects[i]); i++) {
-          fx.fn.call(this, property, value, fx.effect, old, fromAbove);
+          if (!fx.disabled) {
+            fx.fn.call(this, property, value, fx.effect, old, fromAbove);
+          }
         }
       },
 
diff --git a/src/standard/effectBuilder.html b/src/standard/effectBuilder.html
index 07b26be646..87ed23d936 100644
--- a/src/standard/effectBuilder.html
+++ b/src/standard/effectBuilder.html
@@ -73,20 +73,33 @@
       }
 
       // Theoretically the effects must be sorted again, but `function`-effects
-      // are executed last anyway, so we can skip this for now.
+      // are executed last anyway, so we can skip this for now. NOTE that, if
+      // you add a custom effect inside another effect, t.i. the notification-
+      // loop is already running, the newly added effect will not be executed
+      // before the next notification we have to propagate. This is intended
+      // and a bonus point for adding all custom effects at the end.
       return this._addPropertyEffect(property, 'function', fn);
     },
 
     removeCustomEffect: function(fx) {
-      var property = fx.trigger;
-      var effects = property && this._propertyEffects &&
-                    this._propertyEffects[property];
-      if (effects) {
-        var index = effects.indexOf(fx);
-        if (index !== -1) {
-          effects.splice(index, 1);
+      // Since effects can be removed as a side-effect, t.i. within a running
+      // effects-loop, we must ensure not to change the effects- array
+      // immediately, otherwise the for-loop would break. On the other side
+      // removing an effect must be immediate too. So we use a flag here, and
+      // actually remove the effect in a microtask.
+      fx.disabled = true;
+
+      this.async(function() {
+        var property = fx.trigger;
+        var effects = property && this._propertyEffects &&
+                      this._propertyEffects[property];
+        if (effects) {
+          var index = effects.indexOf(fx);
+          if (index !== -1) {
+            effects.splice(index, 1);
+          }
         }
-      }
+      });
     },
 
     // prototyping
diff --git a/src/standard/notify-path.html b/src/standard/notify-path.html
index 2102aa0480..f25f2f1dea 100644
--- a/src/standard/notify-path.html
+++ b/src/standard/notify-path.html
@@ -274,10 +274,12 @@
         var fx$ = this._propertyEffects && this._propertyEffects[model];
         if (fx$) {
           for (var i=0, fx; (i<fx$.length) && (fx=fx$[i]); i++) {
-            // use memoized path functions
-            var fxFn = fx.pathFn;
-            if (fxFn) {
-              fxFn.call(this, path, value, fx.effect);
+            if (!fx.disabled) {
+              // use memoized path functions
+              var fxFn = fx.pathFn;
+              if (fxFn) {
+                fxFn.call(this, path, value, fx.effect);
+              }
             }
           }
         }
diff --git a/test/unit/bind.html b/test/unit/bind.html
index ff0f46efac..d3d0fe0dff 100644
--- a/test/unit/bind.html
+++ b/test/unit/bind.html
@@ -1122,6 +1122,47 @@
     assert.equal(called, 1);
   });
 
+  test('Removing an effect as a side-effect does not break the running ' +
+       'effect loop', function() {
+    var el = document.createElement('x-custom-effect');
+
+    var called = 0;
+    var fx = el.addCustomEffect('foo', function() {
+      el.removeCustomEffect(fx);
+    });
+    el.addCustomEffect('foo', function() {
+      called += 1;
+    });
+
+    el.foo = 'bar';
+    assert.equal(called, 1);
+  });
+
+  test('Adding an effect as a side-effect does not invoke it immediately',
+       function() {
+    var el = document.createElement('x-custom-effect');
+
+    var called = 0;
+    var firstRun = true;
+    el.addCustomEffect('foo', function() {
+      if (firstRun) {
+        el.addCustomEffect('foo', function() {
+          called += 1;
+        });
+        firstRun = false;
+      }
+    });
+
+    el.foo = 'bar';
+    assert.equal(firstRun, false);
+    assert.equal(called, 0);
+
+    el.foo = 'quod';
+    assert.equal(called, 1);
+
+
+  });
+
 
 });