From 57af15ce8b9074334d9f0d3a04bbfb0acef4976f Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Wed, 18 Jan 2017 00:46:43 -0500 Subject: [PATCH] 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 | 53 ++++++++++++++++++---------- src/js/tech/flash.js | 2 -- src/js/tech/html5.js | 2 -- src/js/tech/tech.js | 6 ++-- test/unit/component.test.js | 40 ++++++++++++++++++++- test/unit/tracks/text-tracks.test.js | 17 ++++++--- 6 files changed, 89 insertions(+), 31 deletions(-) diff --git a/src/js/component.js b/src/js/component.js index 373df4dc6b..c05e542a4c 100644 --- a/src/js/component.js +++ b/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; diff --git a/src/js/tech/flash.js b/src/js/tech/flash.js index 7ed1662010..dd9ecdff94 100644 --- a/src/js/tech/flash.js +++ b/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; diff --git a/src/js/tech/html5.js b/src/js/tech/html5.js index a6d0d35557..8fc4d37c98 100644 --- a/src/js/tech/html5.js +++ b/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; diff --git a/src/js/tech/tech.js b/src/js/tech/tech.js index c905773fd5..48e07a919d 100644 --- a/src/js/tech/tech.js +++ b/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; diff --git a/test/unit/component.test.js b/test/unit/component.test.js index 20c253c56f..cd011910a9 100644 --- a/test/unit/component.test.js +++ b/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 diff --git a/test/unit/tracks/text-tracks.test.js b/test/unit/tracks/text-tracks.test.js index 452ddd5ac8..fb7e5cb899 100644 --- a/test/unit/tracks/text-tracks.test.js +++ b/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;