refactor: Make registerComponent only work with Components (#3802)

Prevent techs (and others) from being registered via registerComponent.
* `registerComponent` now throws if given an object that is not a subclass of Component (or Component itself).
* `registerComponent` now throws if given a non-empty string as its name argument.

BREAKING CHANGE: registerComponent now throws if no name or not a component is passed in.

src/js/component.js

@@ -1520,15 +1520,34 @@ class Component {
    * @param {string} name
    *        The name of the `Component` to register.
    *
-   * @param {Component} comp
+   * @param {Component} ComponentToRegister
    *        The `Component` class to register.
    *
    * @return {Component}
    *         The `Component` that was registered.
    */
-  static registerComponent(name, comp) {
-    if (!name) {
-      return;
+  static registerComponent(name, ComponentToRegister) {
+    if (typeof name !== 'string' || !name) {
+      throw new Error(`Illegal component name, "${name}"; must be a non-empty string.`);
+    }
+
+    const Tech = Component.getComponent('Tech');
+
+    // We need to make sure this check is only done if Tech has been registered.
+    const isTech = Tech && Tech.isTech(ComponentToRegister);
+    const isComp = Component === ComponentToRegister ||
+      Component.prototype.isPrototypeOf(ComponentToRegister.prototype);
+
+    if (isTech || !isComp) {
+      let reason;
+
+      if (isTech) {
+        reason = 'techs must be registered using Tech.registerTech()';
+      } else {
+        reason = 'must be a Component subclass';
+      }
+
+      throw new Error(`Illegal component, "${name}"; ${reason}.`);
     }
 
     name = toTitleCase(name);
@@ -1537,23 +1556,26 @@ class Component {
       Component.components_ = {};
     }
 
-    if (name === 'Player' && Component.components_[name]) {
-      const Player = Component.components_[name];
+    const Player = Component.getComponent('Player');
+
+    if (name === 'Player' && Player) {
+      const players = Player.players;
+      const playerNames = Object.keys(players);
 
       // If we have players that were disposed, then their name will still be
       // in Players.players. So, we must loop through and verify that the value
       // for each item is not null. This allows registration of the Player component
       // after all players have been disposed or before any were created.
-      if (Player.players &&
-          Object.keys(Player.players).length > 0 &&
-          Object.keys(Player.players).map((playerName) => Player.players[playerName]).every(Boolean)) {
-        throw new Error('Can not register Player component after player has been created');
+      if (players &&
+          playerNames.length > 0 &&
+          playerNames.map((pname) => players[pname]).every(Boolean)) {
+        throw new Error('Can not register Player component after player has been created.');
       }
     }
 
-    Component.components_[name] = comp;
+    Component.components_[name] = ComponentToRegister;
 
-    return comp;
+    return ComponentToRegister;
   }
 
   /**
@@ -1580,14 +1602,9 @@ class Component {
     if (Component.components_ && Component.components_[name]) {
       return Component.components_[name];
     }
-
-    if (window && window.videojs && window.videojs[name]) {
-      log.warn(`The ${name} component was added to the videojs object when it should be registered using videojs.registerComponent(name, component)`);
-
-      return window.videojs[name];
-    }
   }
 }
 
 Component.registerComponent('Component', Component);
+
 export default Component;

src/js/tech/flash.js

@@ -10,7 +10,6 @@ import * as Dom from '../utils/dom.js';
 import * as Url from '../utils/url.js';
 import { createTimeRange } from '../utils/time-ranges.js';
 import FlashRtmpDecorator from './flash-rtmp';
-import Component from '../component';
 import window from 'global/window';
 import {assign} from '../utils/obj';
 
@@ -1075,6 +1074,5 @@ Flash.getEmbedCode = function(swf, flashVars, params, attributes) {
 // Run Flash through the RTMP decorator
 FlashRtmpDecorator(Flash);
 
-Component.registerComponent('Flash', Flash);
 Tech.registerTech('Flash', Flash);
 export default Flash;

src/js/tech/html5.js

@@ -2,7 +2,6 @@
  * @file html5.js
  */
 import Tech from './tech.js';
-import Component from '../component';
 import * as Dom from '../utils/dom.js';
 import * as Url from '../utils/url.js';
 import * as Fn from '../utils/fn.js';
@@ -1662,6 +1661,5 @@ Html5.nativeSourceHandler.dispose = function() {};
 // Register the native source handler
 Html5.registerSourceHandler(Html5.nativeSourceHandler);
 
-Component.registerComponent('Html5', Html5);
 Tech.registerTech('Html5', Html5);
 export default Html5;

src/js/tech/tech.js

@@ -1178,9 +1178,9 @@ Tech.withSourceHandlers = function(_Tech) {
 
 };
 
+// The base Tech class needs to be registered as a Component. It is the only
+// Tech that can be registered as a Component.
 Component.registerComponent('Tech', Tech);
-// Old name for Tech
-// @deprecated
-Component.registerComponent('MediaTechController', Tech);
 Tech.registerTech('Tech', Tech);
+
 export default Tech;

test/unit/component.test.js

@@ -41,6 +41,44 @@ const getFakePlayer = function() {
   };
 };
 
+QUnit.test('registerComponent() throws with bad arguments', function(assert) {
+  assert.throws(
+    function() {
+      Component.registerComponent(null);
+    },
+    new Error('Illegal component name, "null"; must be a non-empty string.'),
+    'component names must be non-empty strings'
+  );
+
+  assert.throws(
+    function() {
+      Component.registerComponent('');
+    },
+    new Error('Illegal component name, ""; must be a non-empty string.'),
+    'component names must be non-empty strings'
+  );
+
+  assert.throws(
+    function() {
+      Component.registerComponent('TestComponent5', function() {});
+    },
+    new Error('Illegal component, "TestComponent5"; must be a Component subclass.'),
+    'components must be subclasses of Component'
+  );
+
+  assert.throws(
+    function() {
+      const Tech = Component.getComponent('Tech');
+
+      class DummyTech extends Tech {}
+
+      Component.registerComponent('TestComponent5', DummyTech);
+    },
+    new Error('Illegal component, "TestComponent5"; techs must be registered using Tech.registerTech().'),
+    'components must be subclasses of Component'
+  );
+});
+
 QUnit.test('should create an element', function(assert) {
   const comp = new Component(getFakePlayer(), {});
 
@@ -635,7 +673,7 @@ QUnit.test('should use a defined content el for appending children', function(as
   class CompWithContent extends Component {}
 
   CompWithContent.prototype.createEl = function() {
-    // Create the main componenent element
+    // Create the main component element
     const el = Dom.createEl('div');
 
     // Create the element where children will be appended

test/unit/tracks/text-tracks.test.js

@@ -213,7 +213,6 @@ QUnit.test('if native text tracks are not supported, create a texttrackdisplay',
   const oldTestVid = Html5.TEST_VID;
   const oldIsFirefox = browser.IS_FIREFOX;
   const oldTextTrackDisplay = Component.getComponent('TextTrackDisplay');
-  let called = false;
   const tag = document.createElement('video');
   const track1 = document.createElement('track');
   const track2 = document.createElement('track');
@@ -235,13 +234,21 @@ QUnit.test('if native text tracks are not supported, create a texttrackdisplay',
   };
 
   browser.IS_FIREFOX = true;
-  Component.registerComponent('TextTrackDisplay', function() {
-    called = true;
-  });
+
+  const fakeTTDSpy = sinon.spy();
+
+  class FakeTTD extends Component {
+    constructor() {
+      super();
+      fakeTTDSpy();
+    }
+  }
+
+  Component.registerComponent('TextTrackDisplay', FakeTTD);
 
   const player = TestHelpers.makePlayer({}, tag);
 
-  assert.ok(called, 'text track display was created');
+  assert.strictEqual(fakeTTDSpy.callCount, 1, 'text track display was created');
 
   Html5.TEST_VID = oldTestVid;
   browser.IS_FIREFOX = oldIsFirefox;