Merge pull request #18774 from emberjs/bugfix/fix-property-changes

[BUGFIX] Suspend observer deactivation during property changes

packages/@ember/-internals/metal/lib/observer.ts

@@ -107,7 +107,15 @@ export function activateObserver(target: object, eventName: string, sync = false
   }
 }
 
+let DEACTIVATE_SUSPENDED = false;
+let SCHEDULED_DEACTIVATE: [object, string, boolean][] = [];
+
 export function deactivateObserver(target: object, eventName: string, sync = false) {
+  if (DEACTIVATE_SUSPENDED === true) {
+    SCHEDULED_DEACTIVATE.push([target, eventName, sync]);
+    return;
+  }
+
   let observerMap = sync === true ? SYNC_OBSERVERS : ASYNC_OBSERVERS;
 
   let activeObservers = observerMap.get(target);
@@ -127,6 +135,20 @@ export function deactivateObserver(target: object, eventName: string, sync = fal
   }
 }
 
+export function suspendedObserverDeactivation() {
+  DEACTIVATE_SUSPENDED = true;
+}
+
+export function resumeObserverDeactivation() {
+  DEACTIVATE_SUSPENDED = false;
+
+  for (let [target, eventName, sync] of SCHEDULED_DEACTIVATE) {
+    deactivateObserver(target, eventName, sync);
+  }
+
+  SCHEDULED_DEACTIVATE = [];
+}
+
 /**
  * Primarily used for cases where we are redefining a class, e.g. mixins/reopen
  * being applied later. Revalidates all the observers, resetting their tags.

packages/@ember/-internals/metal/lib/property_events.ts

@@ -1,6 +1,10 @@
 import { Meta, peekMeta } from '@ember/-internals/meta';
 import { symbol } from '@ember/-internals/utils';
-import { flushSyncObservers } from './observer';
+import {
+  flushSyncObservers,
+  resumeObserverDeactivation,
+  suspendedObserverDeactivation,
+} from './observer';
 import { markObjectAsDirty } from './tags';
 
 /**
@@ -54,6 +58,7 @@ function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta | null)
 */
 function beginPropertyChanges(): void {
   deferred++;
+  suspendedObserverDeactivation();
 }
 
 /**
@@ -64,6 +69,7 @@ function endPropertyChanges(): void {
   deferred--;
   if (deferred <= 0) {
     flushSyncObservers();
+    resumeObserverDeactivation();
   }
 }
 

packages/@ember/-internals/metal/tests/observer_test.js

@@ -1,6 +1,7 @@
 import { ENV } from '@ember/-internals/environment';
 import {
   destroy,
+  changeProperties,
   addObserver,
   removeObserver,
   notifyPropertyChange,
@@ -987,3 +988,118 @@ moduleFor(
     }
   }
 );
+
+moduleFor(
+  'changeProperties',
+  class extends AbstractTestCase {
+    afterEach() {
+      if (obj !== undefined) {
+        destroy(obj);
+        obj = undefined;
+        return runLoopSettled();
+      }
+    }
+
+    '@test observers added/removed during changeProperties should do the right thing.'(assert) {
+      obj = {
+        foo: 0,
+      };
+      function Observer() {
+        this.didChangeCount = 0;
+      }
+      Observer.prototype = {
+        add() {
+          addObserver(obj, 'foo', this, 'didChange');
+        },
+        remove() {
+          removeObserver(obj, 'foo', this, 'didChange');
+        },
+        didChange() {
+          this.didChangeCount++;
+        },
+      };
+      let addedBeforeFirstChangeObserver = new Observer();
+      let addedAfterFirstChangeObserver = new Observer();
+      let addedAfterLastChangeObserver = new Observer();
+      let removedBeforeFirstChangeObserver = new Observer();
+      let removedBeforeLastChangeObserver = new Observer();
+      let removedAfterLastChangeObserver = new Observer();
+      removedBeforeFirstChangeObserver.add();
+      removedBeforeLastChangeObserver.add();
+      removedAfterLastChangeObserver.add();
+      changeProperties(function() {
+        removedBeforeFirstChangeObserver.remove();
+        addedBeforeFirstChangeObserver.add();
+
+        set(obj, 'foo', 1);
+
+        assert.equal(
+          addedBeforeFirstChangeObserver.didChangeCount,
+          0,
+          'addObserver called before the first change is deferred'
+        );
+
+        addedAfterFirstChangeObserver.add();
+        removedBeforeLastChangeObserver.remove();
+
+        set(obj, 'foo', 2);
+
+        assert.equal(
+          addedAfterFirstChangeObserver.didChangeCount,
+          0,
+          'addObserver called after the first change is deferred'
+        );
+
+        addedAfterLastChangeObserver.add();
+        removedAfterLastChangeObserver.remove();
+      });
+
+      assert.equal(
+        removedBeforeFirstChangeObserver.didChangeCount,
+        0,
+        'removeObserver called before the first change sees none'
+      );
+      assert.equal(
+        addedBeforeFirstChangeObserver.didChangeCount,
+        1,
+        'addObserver called before the first change sees only 1'
+      );
+      assert.equal(
+        addedAfterFirstChangeObserver.didChangeCount,
+        1,
+        'addObserver called after the first change sees 1'
+      );
+      assert.equal(
+        addedAfterLastChangeObserver.didChangeCount,
+        1,
+        'addObserver called after the last change sees 1'
+      );
+      assert.equal(
+        removedBeforeLastChangeObserver.didChangeCount,
+        0,
+        'removeObserver called before the last change sees none'
+      );
+      assert.equal(
+        removedAfterLastChangeObserver.didChangeCount,
+        0,
+        'removeObserver called after the last change sees none'
+      );
+    }
+
+    '@test calling changeProperties while executing deferred observers works correctly'(assert) {
+      obj = { foo: 0 };
+      let fooDidChange = 0;
+
+      addObserver(obj, 'foo', () => {
+        fooDidChange++;
+        changeProperties(() => {});
+      });
+
+      changeProperties(() => {
+        set(obj, 'foo', 1);
+      });
+
+      assert.equal(fooDidChange, 1);
+    }
+  }
+);