fix: better evented validation and error messages (#6982)

src/js/mixins/evented.js

@@ -9,6 +9,26 @@ import * as Fn from '../utils/fn';
 import * as Obj from '../utils/obj';
 import EventTarget from '../event-target';
 
+const objName = (obj) => {
+  if (typeof obj.name === 'function') {
+    return obj.name();
+  }
+
+  if (typeof obj.name === 'string') {
+    return obj.name;
+  }
+
+  if (obj.name_) {
+    return obj.name_;
+  }
+
+  if (obj.constructor && obj.constructor.name) {
+    return obj.constructor.name;
+  }
+
+  return typeof obj;
+};
+
 /**
  * Returns whether or not an object has had the evented mixin applied.
  *
@@ -67,10 +87,16 @@ const isValidEventType = (type) =>
  *
  * @param  {Object} target
  *         The object to test.
+ *
+ * @param  {Object} obj
+ *         The evented object we are validating for
+ *
+ * @param  {string} fnName
+ *         The name of the evented mixin function that called this.
  */
-const validateTarget = (target) => {
-  if (!target.nodeName && !isEvented(target)) {
-    throw new Error('Invalid target; must be a DOM node or evented object.');
+const validateTarget = (target, obj, fnName) => {
+  if (!target || (!target.nodeName && !isEvented(target))) {
+    throw new Error(`Invalid target for ${objName(obj)}#${fnName}; must be a DOM node or evented object.`);
   }
 };
 
@@ -83,10 +109,16 @@ const validateTarget = (target) => {
  *
  * @param  {string|Array} type
  *         The type to test.
+ *
+ * @param  {Object} obj
+*         The evented object we are validating for
+ *
+ * @param  {string} fnName
+ *         The name of the evented mixin function that called this.
  */
-const validateEventType = (type) => {
+const validateEventType = (type, obj, fnName) => {
   if (!isValidEventType(type)) {
-    throw new Error('Invalid event type; must be a non-empty string or array.');
+    throw new Error(`Invalid event type for ${objName(obj)}#${fnName}; must be a non-empty string or array.`);
   }
 };
 
@@ -99,10 +131,16 @@ const validateEventType = (type) => {
  *
  * @param  {Function} listener
  *         The listener to test.
+ *
+ * @param  {Object} obj
+ *         The evented object we are validating for
+ *
+ * @param  {string} fnName
+ *         The name of the evented mixin function that called this.
  */
-const validateListener = (listener) => {
+const validateListener = (listener, obj, fnName) => {
   if (typeof listener !== 'function') {
-    throw new Error('Invalid listener; must be a function.');
+    throw new Error(`Invalid listener for ${objName(obj)}#${fnName}; must be a function.`);
   }
 };
 
@@ -118,10 +156,13 @@ const validateListener = (listener) => {
  * @param  {Array} args
  *         An array of arguments passed to `on()` or `one()`.
  *
+ * @param  {string} fnName
+ *         The name of the evented mixin function that called this.
+ *
  * @return {Object}
  *         An object containing useful values for `on()` or `one()` calls.
  */
-const normalizeListenArgs = (self, args) => {
+const normalizeListenArgs = (self, args, fnName) => {
 
   // If the number of arguments is less than 3, the target is always the
   // evented object itself.
@@ -144,9 +185,9 @@ const normalizeListenArgs = (self, args) => {
     [target, type, listener] = args;
   }
 
-  validateTarget(target);
-  validateEventType(type);
-  validateListener(listener);
+  validateTarget(target, self, fnName);
+  validateEventType(type, self, fnName);
+  validateListener(listener, self, fnName);
 
   listener = Fn.bind(self, listener);
 
@@ -171,7 +212,7 @@ const normalizeListenArgs = (self, args) => {
  *         A listener function.
  */
 const listen = (target, method, type, listener) => {
-  validateTarget(target);
+  validateTarget(target, target, method);
 
   if (target.nodeName) {
     Events[method](target, type, listener);
@@ -212,7 +253,7 @@ const EventedMixin = {
    *         the listener function.
    */
   on(...args) {
-    const {isTargetingSelf, target, type, listener} = normalizeListenArgs(this, args);
+    const {isTargetingSelf, target, type, listener} = normalizeListenArgs(this, args, 'on');
 
     listen(target, 'on', type, listener);
 
@@ -264,7 +305,7 @@ const EventedMixin = {
    *         the listener function.
    */
   one(...args) {
-    const {isTargetingSelf, target, type, listener} = normalizeListenArgs(this, args);
+    const {isTargetingSelf, target, type, listener} = normalizeListenArgs(this, args, 'one');
 
     // Targeting this evented object.
     if (isTargetingSelf) {
@@ -313,7 +354,7 @@ const EventedMixin = {
    *         the listener function.
    */
   any(...args) {
-    const {isTargetingSelf, target, type, listener} = normalizeListenArgs(this, args);
+    const {isTargetingSelf, target, type, listener} = normalizeListenArgs(this, args, 'any');
 
     // Targeting this evented object.
     if (isTargetingSelf) {
@@ -364,9 +405,9 @@ const EventedMixin = {
       const type = typeOrListener;
 
       // Fail fast and in a meaningful way!
-      validateTarget(target);
-      validateEventType(type);
-      validateListener(listener);
+      validateTarget(target, this, 'off');
+      validateEventType(type, this, 'off');
+      validateListener(listener, this, 'off');
 
       // Ensure there's at least a guid, even if the function hasn't been used
       listener = Fn.bind(this, listener);
@@ -398,6 +439,14 @@ const EventedMixin = {
    *          Whether or not the default behavior was prevented.
    */
   trigger(event, hash) {
+    validateTarget(this.eventBusEl_, this, 'trigger');
+
+    const type = event && typeof event !== 'string' ? event.type : event;
+
+    if (!isValidEventType(type)) {
+      throw new Error(`Invalid event type for ${objName(this)}#trigger; ` +
+        'must be a non-empty string or object with a type key that has a non-empty value.');
+    }
     return Events.trigger(this.eventBusEl_, event, hash);
   }
 };

test/unit/mixins/evented.test.js

@@ -6,9 +6,10 @@ import * as Obj from '../../../src/js/utils/obj';
 
 // Common errors thrown by evented objects.
 const errors = {
-  type: new Error('Invalid event type; must be a non-empty string or array.'),
-  listener: new Error('Invalid listener; must be a function.'),
-  target: new Error('Invalid target; must be a DOM node or evented object.')
+  type: (objName, fnName) => new Error(`Invalid event type for ${objName}#${fnName}; must be a non-empty string or array.`),
+  listener: (objName, fnName) => new Error(`Invalid listener for ${objName}#${fnName}; must be a function.`),
+  target: (objName, fnName) => new Error(`Invalid target for ${objName}#${fnName}; must be a DOM node or evented object.`),
+  trigger: (objName) => new Error(`Invalid event type for ${objName}#trigger; must be a non-empty string or object with a type key that has a non-empty value.`)
 };
 
 const validateListenerCall = (call, thisValue, eventExpectation) => {
@@ -61,33 +62,76 @@ QUnit.test('evented() with custom element', function(assert) {
   );
 });
 
+QUnit.test('trigger() errors', function(assert) {
+  class Test {}
+  const targeta = evented({});
+  const targetb = evented(new Test());
+  const targetc = evented(new Test());
+
+  targetc.name_ = 'foo';
+
+  [targeta, targetb, targetc].forEach((target) => {
+    const objName = target.name_ || target.constructor.name || typeof target;
+    const triggerError = errors.trigger(objName);
+
+    assert.throws(() => target.trigger(), triggerError, 'expected error');
+    assert.throws(() => target.trigger('   '), triggerError, 'expected error');
+    assert.throws(() => target.trigger({}), triggerError, 'expected error');
+    assert.throws(() => target.trigger({type: ''}), triggerError, 'expected error');
+    assert.throws(() => target.trigger({type: '    '}), triggerError, 'expected error');
+
+    delete target.eventBusEl_;
+
+    assert.throws(() => target.trigger({type: 'foo'}), errors.target(objName, 'trigger'), 'expected error');
+  });
+});
+
 QUnit.test('on(), one(), and any() errors', function(assert) {
+  class Test {}
   const targeta = this.targets.a = evented({});
   const targetb = this.targets.b = evented({});
+  const targetc = this.targets.c = evented(new Test());
+  const targetd = this.targets.d = evented(new Test());
+
+  targetd.name_ = 'foo';
 
   ['on', 'one', 'any'].forEach(method => {
-    assert.throws(() => targeta[method](), errors.type, 'the expected error is thrown');
-    assert.throws(() => targeta[method]('   '), errors.type, 'the expected error is thrown');
-    assert.throws(() => targeta[method]([]), errors.type, 'the expected error is thrown');
-    assert.throws(() => targeta[method]('x'), errors.listener, 'the expected error is thrown');
-    assert.throws(() => targeta[method]({}, 'x', () => {}), errors.target, 'the expected error is thrown');
-    assert.throws(() => targeta[method](targetb, 'x', null), errors.listener, 'the expected error is thrown');
+    [targeta, targetc, targetd].forEach((target) => {
+      const objName = target.name_ || target.constructor.name || typeof target;
+
+      assert.throws(() => target[method](), errors.type(objName, method), 'expected error');
+      assert.throws(() => target[method]('   '), errors.type(objName, method), 'expected error');
+      assert.throws(() => target[method]([]), errors.type(objName, method), 'expected error');
+      assert.throws(() => target[method]('x'), errors.listener(objName, method), 'expected error');
+      assert.throws(() => target[method]({}, 'x', () => {}), errors.target(objName, method), 'expected error');
+      assert.throws(() => target[method](targetb, 'x', null), errors.listener(objName, method), 'expected error');
+    });
   });
 });
 
 QUnit.test('off() errors', function(assert) {
+  class Test {}
   const targeta = this.targets.a = evented({});
   const targetb = this.targets.b = evented({});
   const targetc = this.targets.c = evented({});
   const targetd = this.targets.d = evented({});
+  const targete = this.targets.e = evented(new Test());
+  const targetf = this.targets.f = evented(new Test());
+
+  targetf.name_ = 'foo';
 
   // An invalid event actually causes an invalid target error because it
   // gets passed into code that assumes the first argument is the target.
-  assert.throws(() => targeta.off([]), errors.target, 'the expected error is thrown');
-  assert.throws(() => targeta.off({}, 'x', () => {}), errors.target, 'the expected error is thrown');
-  assert.throws(() => targeta.off(targetb, '', () => {}), errors.type, 'the expected error is thrown');
-  assert.throws(() => targeta.off(targetc, [], () => {}), errors.type, 'the expected error is thrown');
-  assert.throws(() => targeta.off(targetd, 'x', null), errors.listener, 'the expected error is thrown');
+  [targeta, targete, targetf].forEach(function(target) {
+    const objName = target.name_ || target.constructor.name || typeof target;
+
+    assert.throws(() => target.off([]), errors.target(objName, 'off'), 'expected error');
+    assert.throws(() => target.off({}, 'x', () => {}), errors.target(objName, 'off'), 'expected error');
+    assert.throws(() => target.off(targetb, '', () => {}), errors.type(objName, 'off'), 'expected error');
+    assert.throws(() => target.off(targetc, [], () => {}), errors.type(objName, 'off'), 'expected error');
+    assert.throws(() => target.off(targetd, 'x', null), errors.listener(objName, 'off'), 'expected error');
+    assert.throws(() => target.off(targetd, 'x', null), errors.listener(objName, 'off'), 'expected error');
+  });
 });
 
 QUnit.test('on() can add a listener to one event type on this object', function(assert) {