[BUGFIX] Suspend observer deactivation during property changes

We accidentally removed some tests for timing changes in observers when
we were originally refactoring observers. This PR adds these tests back,
and suspends deactivation of observers until after property changes have
completed and observers have been properly fired.

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,6 @@
 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 +54,7 @@ function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta | null)
 */
 function beginPropertyChanges(): void {
   deferred++;
+  suspendedObserverDeactivation();
 }
 
 /**
@@ -64,6 +65,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);
+    }
+  }
+);