@gkatsev made initListeners more general and added Tech.isTech. Fixes #2767. closes #2773

.jshintrc

@@ -34,6 +34,7 @@
         "notEqual",
         "notStrictEqual",
         "ok",
+        "throws",
         "QUnit",
         "raises",
         "start",

CHANGELOG.md

@@ -2,7 +2,7 @@ CHANGELOG
 =========
 
 ## HEAD (Unreleased)
-_(none)_
+* @gkatsev made initListeners more general and added Tech.isTech. Fixes #2767 ([view](https://github.com/videojs/video.js/pull/2773))
 
 --------------------
 

src/js/component.js

@@ -369,6 +369,10 @@ class Component {
       // If there's no .player_, this is a player
       let ComponentClass = Component.getComponent(componentClassName);
 
+      if (!ComponentClass) {
+        throw new Error(`Component ${componentClassName} does not exist`);
+      }
+
       component = new ComponentClass(this.player_ || this, options);
 
     // child is a component instance
@@ -493,7 +497,10 @@ class Component {
       // `this` is `parent`
       let parentOptions = this.options_;
 
-      let handleAdd = (name, opts) => {
+      let handleAdd = (child) => {
+        let name = child.name;
+        let opts = child.opts;
+
         // Allow options for children to be set at the parent options
         // e.g. videojs(id, { controlBar: false });
         // instead of videojs(id, { children: { controlBar: false });
@@ -525,29 +532,50 @@ class Component {
       };
 
       // Allow for an array of children details to passed in the options
+      let workingChildren;
+      let Tech = Component.getComponent('Tech');
+
       if (Array.isArray(children)) {
-        for (let i = 0; i < children.length; i++) {
-          let child = children[i];
-          let name;
-          let opts;
-
-          if (typeof child === 'string') {
-            // ['myComponent']
-            name = child;
-            opts = {};
-          } else {
-            // [{ name: 'myComponent', otherOption: true }]
-            name = child.name;
-            opts = child;
-          }
-
-          handleAdd(name, opts);
-        }
+        workingChildren = children;
       } else {
-        Object.getOwnPropertyNames(children).forEach(function(name){
-          handleAdd(name, children[name]);
-        });
+        workingChildren = Object.keys(children);
       }
+
+      workingChildren
+      // children that are in this.options_ but also in workingChildren  would
+      // give us extra children we do not want. So, we want to filter them out.
+      .concat(Object.keys(this.options_)
+              .filter(function(child) {
+                return !workingChildren.some(function(wchild) {
+                  if (typeof wchild === 'string') {
+                    return child === wchild;
+                  } else {
+                    return child === wchild.name;
+                  }
+                });
+              }))
+      .map((child) => {
+        let name, opts;
+
+        if (typeof child === 'string') {
+          name = child;
+          opts = children[name] || this.options_[name] || {};
+        } else {
+          name = child.name;
+          opts = child;
+        }
+
+        return {name, opts};
+      })
+      .filter((child) => {
+        // we have to make sure that child.name isn't in the techOrder since
+        // techs are registerd as Components but can't aren't compatible
+        // See https://github.com/videojs/video.js/issues/2772
+        let c = Component.getComponent(child.opts.componentClass ||
+                                       toTitleCase(child.name));
+        return c && !Tech.isTech(c);
+      })
+      .forEach(handleAdd);
     }
   }
 

src/js/tech/tech.js

@@ -437,6 +437,18 @@ class Tech extends Component {
     return '';
   }
 
+  /*
+   * Return whether the argument is a Tech or not.
+   * Can be passed either a Class like `Html5` or a instance like `player.tech_`
+   * 
+   * @param {Object} component An item to check
+   * @return {Boolean}         Whether it is a tech or not
+   */
+  static isTech(component) {
+    return component.prototype instanceof Tech ||
+           component instanceof Tech ||
+           component === Tech;
+  }
 }
 
 /*

test/unit/component.test.js

@@ -5,6 +5,21 @@ import * as browser from '../../src/js/utils/browser.js';
 import document from 'global/document';
 import TestHelpers from './test-helpers.js';
 
+class TestComponent1 extends Component {}
+class TestComponent2 extends Component {}
+class TestComponent3 extends Component {}
+class TestComponent4 extends Component {}
+TestComponent1.prototype.options_ = {
+  children: [
+    'testComponent2',
+    'testComponent3'
+  ]
+};
+Component.registerComponent('TestComponent1', TestComponent1);
+Component.registerComponent('TestComponent2', TestComponent2);
+Component.registerComponent('TestComponent3', TestComponent3);
+Component.registerComponent('TestComponent4', TestComponent4);
+
 q.module('Component', {
   'setup': function() {
     this.clock = sinon.useFakeTimers();
@@ -40,6 +55,14 @@ test('should add a child component', function(){
   ok(comp.getChildById(child.id()) === child);
 });
 
+test('addChild should throw if the child does not exist', function() {
+  var comp = new Component(getFakePlayer());
+
+  throws(function() {
+    comp.addChild('non-existent-child');
+  }, new Error('Component Non-existent-child does not exist'), 'addChild threw');
+});
+
 test('should init child components from options', function(){
   var comp = new Component(getFakePlayer(), {
     children: {
@@ -111,6 +134,16 @@ test('should do a deep merge of child options', function(){
   Component.prototype.options_ = null;
 });
 
+test('should init child components from component options', function(){
+  let testComp = new TestComponent1(TestHelpers.makePlayer(), {
+    testComponent2: false,
+    testComponent4: {}
+  });
+
+  ok(!testComp.childNameIndex_.testComponent2, 'we do not have testComponent2');
+  ok(testComp.childNameIndex_.testComponent4, 'we have a testComponent4');
+});
+
 test('should allows setting child options at the parent options level', function(){
   var parent, options;
 
@@ -133,6 +166,7 @@ test('should allows setting child options at the parent options level', function
     ok(false, 'Child with `false` option was initialized');
   }
   equal(parent.children()[0].options_['foo'], true, 'child options set when children array is used');
+  equal(parent.children().length, 1, 'we should only have one child');
 
   // using children object
   options = {
@@ -155,6 +189,7 @@ test('should allows setting child options at the parent options level', function
     ok(false, 'Child with `false` option was initialized');
   }
   equal(parent.children()[0].options_['foo'], true, 'child options set when children object is used');
+  equal(parent.children().length, 1, 'we should only have one child');
 });
 
 test('should dispose of component and children', function(){

test/unit/tech/tech.test.js

@@ -1,6 +1,9 @@
 var noop = function() {}, clock, oldTextTracks;
 
 import Tech from '../../../src/js/tech/tech.js';
+import Html5 from '../../../src/js/tech/html5.js';
+import Flash from '../../../src/js/tech/flash.js';
+import Button from '../../../src/js/button.js';
 import { createTimeRange } from '../../../src/js/utils/time-ranges.js';
 import extendFn from '../../../src/js/extend.js';
 import MediaError from '../../../src/js/media-error.js';
@@ -271,3 +274,17 @@ test('delegates seekable to the source handler', function(){
   tech.seekable();
   equal(seekableCount, 1, 'called the source handler');
 });
+
+test('Tech.isTech returns correct answers for techs and components', function() {
+  let isTech = Tech.isTech;
+
+  ok(isTech(Tech), 'Tech is a Tech');
+  ok(isTech(Html5), 'Html5 is a Tech');
+  ok(isTech(new Html5({}, {})), 'An html5 instance is a Tech');
+  ok(isTech(Flash), 'Flash is a Tech');
+  ok(!isTech(5), 'A number is not a Tech');
+  ok(!isTech('this is a tech'), 'A string is not a Tech');
+  ok(!isTech(Button), 'A Button is not a Tech');
+  ok(!isTech(new Button({}, {})), 'A Button instance is not a Tech');
+  ok(!isTech(isTech), 'A function is not a Tech');
+});