[WIP] Add listener/watcher/observer assertions, many bug fixes

packages/ember-metal/lib/meta_listeners.js

@@ -1,3 +1,5 @@
+import { assert, runInDebug } from 'ember-metal/debug';
+
 /*
  When we render a rich template hierarchy, the set of events that
  *might* happen tends to be much larger than the set of events that
@@ -20,6 +22,24 @@ export var protoMethods = {
     if (!this._listeners) {
       this._listeners = [];
     }
+
+    runInDebug(() => {
+      // Assert that an observer is only used once.
+      let listeners = this._listeners;
+      let matchFound = false;
+
+      for (let i = listeners.length - 4; i >= 0; i -= 4) {
+        if (listeners[i+1] === target &&
+            listeners[i+2] === method &&
+            listeners[i] === eventName) {
+          matchFound = true;
+          break;
+        }
+      }
+
+      assert(`Tried to add a duplicate listener for ${eventName}`, !matchFound);
+    });
+
     this._listeners.push(eventName, target, method, flags);
   },
 
@@ -39,6 +59,7 @@ export var protoMethods = {
   },
 
   removeFromListeners(eventName, target, method, didRemove) {
+    let matchFound = false;
     let pointer = this;
     while (pointer) {
       let listeners = pointer._listeners;
@@ -51,19 +72,23 @@ export var protoMethods = {
                 didRemove(eventName, target, listeners[index + 2]);
               }
               listeners.splice(index, 4);
+              matchFound = true;
             } else {
               // we are trying to remove an inherited listener, so we do
               // just-in-time copying to detach our own listeners from
               // our inheritance chain.
               this._finalizeListeners();
-              return this.removeFromListeners(eventName, target, method);
+              this.removeFromListeners(eventName, target, method);
+              return;
             }
           }
         }
       }
       if (pointer._listenersFinalized) { break; }
       pointer = pointer.parent;
     }
+
+    assert(`Tried to remove a non-existant listener for ${eventName}`, matchFound);
   },
 
   matchingListeners(eventName) {

packages/ember-metal/lib/streams/key-stream.js

@@ -63,7 +63,7 @@ export default BasicStream.extend({
     if (object !== this.observedObject) {
       this._clearObservedObject();
 
-      if (object && typeof object === 'object') {
+      if (typeof object === 'object' && object !== null && !object.isDestroyed) {
         addObserver(object, this.key, this, this.notify);
         this.observedObject = object;
       }
@@ -73,10 +73,11 @@ export default BasicStream.extend({
   _super$deactivate: BasicStream.prototype.deactivate,
 
   _clearObservedObject() {
-    if (this.observedObject) {
-      removeObserver(this.observedObject, this.key, this, this.notify);
-      this.observedObject = null;
+    let observedObject = this.observedObject;
+    if (observedObject !== null && !observedObject.isDestroyed) {
+      removeObserver(observedObject, this.key, this, this.notify);
     }
+    this.observedObject = null;
   },
 
   deactivate() {

packages/ember-metal/lib/watch_key.js

@@ -1,7 +1,6 @@
 import isEnabled from 'ember-metal/features';
-import {
-  meta as metaFor
-} from 'ember-metal/meta';
+import { assert } from 'ember-metal/debug';
+import { meta as metaFor } from 'ember-metal/meta';
 import {
   MANDATORY_SETTER_FUNCTION,
   DEFAULT_GETTER_FUNCTION,
@@ -79,8 +78,14 @@ if (isEnabled('mandatory-setter')) {
 }
 
 export function unwatchKey(obj, keyName, meta) {
+  // can't unwatch length on Array - it is special...
+  if (keyName === 'length' && Array.isArray(obj)) { return; }
+
   var m = meta || metaFor(obj);
   let count = m.peekWatching(keyName);
+
+  assert(`Tried to unwatch '${keyName}' on object but no one was watching`, count > 0);
+
   if (count === 1) {
     m.writeWatching(keyName, 0);
 

packages/ember-metal/lib/watch_path.js

@@ -1,6 +1,5 @@
-import {
-  meta as metaFor
-} from 'ember-metal/meta';
+import { assert } from 'ember-metal/debug';
+import { meta as metaFor } from 'ember-metal/meta';
 import { ChainNode } from 'ember-metal/chains';
 
 // get the chains for the current object. If the current object has
@@ -29,9 +28,14 @@ export function watchPath(obj, keyPath, meta) {
 }
 
 export function unwatchPath(obj, keyPath, meta) {
+  // can't unwatch length on Array - it is special...
+  if (keyPath === 'length' && Array.isArray(obj)) { return; }
+
   var m = meta || metaFor(obj);
   let counter = m.peekWatching(keyPath) || 0;
 
+  assert(`Tried to unwatch '${keyPath}' on object but no one was watching`, counter > 0);
+
   if (counter === 1) {
     m.writeWatching(keyPath, 0);
     chainsFor(obj, m).remove(keyPath);

packages/ember-metal/tests/binding/connect_test.js

@@ -90,7 +90,7 @@ testBoth('Connecting a binding to path', function(get, set) {
   equal(get(a, 'foo'), 'BIFF', 'a should have changed');
 });
 
-testBoth('Calling connect more than once', function(get, set) {
+testBoth('Calling connect more than once throws an error', function(get, set) {
   var b = { bar: 'BAR' };
   var a = { foo: 'FOO', b: b };
 
@@ -100,7 +100,9 @@ testBoth('Calling connect more than once', function(get, set) {
   performTest(binding, a, b, get, set, function () {
     binding.connect(a);
 
-    binding.connect(a);
+    throws(() => {
+      binding.connect(a);
+    }, /Tried to add.*b.bar/);
   });
 });
 

packages/ember-metal/tests/events_test.js

@@ -56,15 +56,15 @@ QUnit.test('listeners should be inherited', function() {
 });
 
 
-QUnit.test('adding a listener more than once should only invoke once', function() {
+QUnit.test('adding a listener more than once throws an error', function() {
   var obj = {};
   var count = 0;
   var F = function() { count++; };
   addListener(obj, 'event!', F);
-  addListener(obj, 'event!', F);
 
-  sendEvent(obj, 'event!');
-  equal(count, 1, 'should only invoke once');
+  throws(function() {
+    addListener(obj, 'event!', F);
+  }, /Tried to add a duplicate listener for event!/);
 });
 
 QUnit.test('adding a listener with a target should invoke with target', function() {
@@ -196,7 +196,9 @@ QUnit.test('while suspended, it should not be possible to add a duplicate listen
   addListener(obj, 'event!', target, target.method);
 
   function callback() {
-    addListener(obj, 'event!', target, target.method);
+    throws(function() {
+      addListener(obj, 'event!', target, target.method);
+    }, /Tried to add a duplicate listener for event!/);
   }
 
   sendEvent(obj, 'event!');

packages/ember-metal/tests/keys_test.js

@@ -107,13 +107,8 @@ QUnit.test('observers switched on and off with setter in between (observed prope
   var yetAnotherBeer = new Beer();
   addObserver(yetAnotherBeer, 'type', K);
   set(yetAnotherBeer, 'type', 'ale');
-  removeObserver(beer, 'type', K);
+  removeObserver(yetAnotherBeer, 'type', K);
   deepEqual(Object.keys(yetAnotherBeer), ['type'], 'addObserver -> set -> removeOjbserver');
-
-  var itsMyLastBeer = new Beer();
-  set(itsMyLastBeer, 'type', 'ale');
-  removeObserver(beer, 'type', K);
-  deepEqual(Object.keys(itsMyLastBeer), ['type'], 'set -> removeObserver');
 });
 
 QUnit.test('observers switched on and off with setter in between (observed property is shadowing one on the prototype)', function () {
@@ -132,13 +127,8 @@ QUnit.test('observers switched on and off with setter in between (observed prope
   var yetAnotherBeer = new Beer();
   addObserver(yetAnotherBeer, 'type', K);
   set(yetAnotherBeer, 'type', 'ale');
-  removeObserver(beer, 'type', K);
+  removeObserver(yetAnotherBeer, 'type', K);
   deepEqual(Object.keys(yetAnotherBeer), ['type'], 'addObserver -> set -> removeObserver');
-
-  var itsMyLastBeer = new Beer();
-  set(itsMyLastBeer, 'type', 'ale');
-  removeObserver(beer, 'type', K);
-  deepEqual(Object.keys(itsMyLastBeer), ['type'], 'set -> removeObserver');
 });
 
 QUnit.test('observers switched on and off with setter in between', function () {

packages/ember-metal/tests/meta_test.js

@@ -66,12 +66,19 @@ QUnit.test('meta.listeners inheritance', function(assert) {
   assert.equal(matching.length, 3);
 });
 
-QUnit.test('meta.listeners deduplication', function(assert) {
+QUnit.test('meta.listeners adding an existing listener throws an error', function(assert) {
   let t = {};
   let m = meta({});
   m.addToListeners('hello', t, 'm', 0);
-  m.addToListeners('hello', t, 'm', 0);
-  let matching = m.matchingListeners('hello');
-  assert.equal(matching.length, 3);
-  assert.equal(matching[0], t);
+  throws(() => {
+    m.addToListeners('hello', t, 'm', 0);
+  }, /Tried to add a duplicate listener for hello/);
+});
+
+QUnit.test('meta.listeners removing a non-existing listener throws an error', function(assert) {
+  let t = {};
+  let m = meta({});
+  throws(() => {
+    m.removeFromListeners('hello', t, 'm');
+  }, /Tried to remove a non-existant listener for hello/);
 });

packages/ember-metal/tests/observer_test.js

@@ -1166,3 +1166,29 @@ testBoth('observer switched on and off and then setter', function (get, set) {
 
   deepEqual(Object.keys(beer), ['type']);
 });
+
+testBoth('adding the same observer multiple times throws an error', function (get, set) {
+  function Beer() { }
+  Beer.prototype.type = 'ipa';
+
+  let beer = new Beer();
+
+  addObserver(beer, 'type', K);
+
+  throws(() => {
+    addObserver(beer, 'type', K);
+  }, /Tried to add.*type/);
+});
+
+testBoth('removing a non-existant observer throws an error', function (get, set) {
+  function Beer() { }
+  Beer.prototype.type = 'ipa';
+
+  let beer = new Beer();
+
+  addObserver(beer, 'type', function() {});
+
+  throws(() => {
+    removeObserver(beer, 'type', K);
+  }, /Tried to remove.*type/);
+});

packages/ember-metal/tests/watching/is_watching_test.js

@@ -1,10 +1,6 @@
 import { computed } from 'ember-metal/computed';
 import { get } from 'ember-metal/property_get';
 import { defineProperty } from 'ember-metal/properties';
-import {
-  Mixin,
-  observer
-} from 'ember-metal/mixin';
 import {
   addObserver,
   removeObserver
@@ -25,16 +21,6 @@ function testObserver(setup, teardown, key) {
   equal(isWatching(obj, key), false, 'isWatching is false after observers are removed');
 }
 
-QUnit.test('isWatching is true for regular local observers', function() {
-  testObserver(function(obj, key, fn) {
-    Mixin.create({
-      didChange: observer(key, fn)
-    }).apply(obj);
-  }, function(obj, key, fn) {
-    removeObserver(obj, key, obj, fn);
-  });
-});
-
 QUnit.test('isWatching is true for nonlocal observers', function() {
   testObserver(function(obj, key, fn) {
     addObserver(obj, key, obj, fn);