Make registerComponent only work with Components #3802

misteroneill · 2016-11-26T19:37:37Z

Description

This addresses one of the pre-existing cards from the 6.0 project. It prevents techs (and others) from being registered via registerComponent.

There is an open question as to whether or not we want to implement similar protections to registerTech and adopt deregisterComponent and deregisterTech methods similar to the way the plugins PR does for plugins.

Specific Changes proposed

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.

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

brandonocasey · 2016-11-28T20:10:44Z

src/js/component.js

    }

-    name = toTitleCase(name);
+    if (!Component.prototype.isPrototypeOf(Comp.prototype) && Component !== Comp) {
+      throw new Error('illegal "Comp"; must be a subclass of Component');


I think this error is a little vague. Could be more like "The component ${name} that was ordered for registration, was not a subclass of Component. This is required.

I tend to prefer shorter errors, but perhaps the part before the semicolon could be clearer.

brandonocasey · 2016-11-28T20:12:22Z

src/js/component.js

-    if (!name) {
-      return;
+  static registerComponent(name, Comp) {
+    if (typeof name !== 'string' || !(/\S/).test(name)) {


will this match empty string? EDIT: it may be a good idea just to comment what that regex matches

Sure. It may be overkill to even have this regex, though. I dunno. I am sometimes accused of being over-defensive in my coding style. 😄

It matches a string with any non-space character. The opposite of \s.

brandonocasey · 2016-11-28T20:12:58Z

src/js/component.js

+      throw new Error('illegal "Comp"; must be a subclass of Component');
+    }
+
+    name = toTitleCase(name.trim());


Should this go at the top? Then we would just have to check for empty string?

it might be worth warning that the component name contains trailing/leading spaces and that we are trimming it here?

I probably shouldn't trim it honestly. But I'll see about moving it up.

I am removing the trim().

misteroneill · 2016-12-02T15:48:27Z

src/js/component.js

-      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];
-    }


Removed this section since it was deprecated in 5.x.

brandonocasey · 2016-12-02T18:04:43Z

src/js/component.js

-      return;
+  static registerComponent(name, ComponentToRegister) {
+    if (typeof name !== 'string' || !name.length) {
+      throw new Error(`illegal component name, "${name}"; must be a non-empty string`);


won't this throw an "uncaught reference error" or something like that if name is undefined?

That's a good point.

Actually, no, it shouldn't. It will only get to the second part of the expression if it's a string.

gkatsev · 2016-12-29T17:17:36Z

src/js/component.js

-  static registerComponent(name, comp) {
-    if (!name) {
-      return;
+  static registerComponent(name, ComponentToRegister) {


I think we also want to check if comp is a Tech and throw an error in that case.

gkatsev · 2017-01-04T16:27:12Z

src/js/component.js

@@ -1553,22 +1572,22 @@ class Component {
    }

    if (name === 'Player' && Component.components_[name]) {
-      const Player = Component.components_[name];
+      const players = Component.components_[name].players;


is it guaranteed that Component.components_[name] exists? I know when I added the check below it wasn't.

It's not, but it's checked above in line 1574. I guess that could be clearer. Will update.

brandonocasey · 2017-01-06T17:19:24Z

src/js/component.js

+      throw new Error(`Illegal component name, "${name}"; must be a non-empty string.`);
+    }
+
+    const Tech = Component.getComponent('Tech');


should we use this or import? other that that this LGTM

Tech depends on Component, so import is out of the question unfortunately.

ok makes sense

misteroneill added the needs: discussion Needs a broader discussion label Nov 26, 2016

misteroneill changed the title ~~registerComponent only works with Components~~ Make registerComponent only work with Components Nov 28, 2016

brandonocasey requested changes Nov 28, 2016

View reviewed changes

misteroneill added the major label Dec 2, 2016

misteroneill commented Dec 2, 2016

View reviewed changes

misteroneill removed the needs: discussion Needs a broader discussion label Dec 2, 2016

brandonocasey reviewed Dec 2, 2016

View reviewed changes

brandonocasey added the needs: addressing label Dec 6, 2016

misteroneill force-pushed the techs-not-components branch from 4c90d96 to e07ae95 Compare December 20, 2016 21:36

misteroneill removed the needs: addressing label Dec 20, 2016

misteroneill force-pushed the techs-not-components branch from e07ae95 to bedf76b Compare December 24, 2016 15:21

gkatsev requested changes Dec 29, 2016

View reviewed changes

misteroneill added 3 commits January 4, 2017 11:17

registerComponent only works with Components

037327e

Address concerns from review

44655ae

Throw when attempting to register a tech as a component

8b717c7

misteroneill force-pushed the techs-not-components branch from bedf76b to 8b717c7 Compare January 4, 2017 16:18

gkatsev reviewed Jan 4, 2017

View reviewed changes

Clarify some code

f611025

brandonocasey reviewed Jan 6, 2017

View reviewed changes

brandonocasey added the needs: discussion Needs a broader discussion label Jan 6, 2017

brandonocasey approved these changes Jan 6, 2017

View reviewed changes

misteroneill removed the needs: discussion Needs a broader discussion label Jan 6, 2017

gkatsev approved these changes Jan 6, 2017

View reviewed changes

misteroneill added the confirmed label Jan 9, 2017

gkatsev merged commit 57af15c into videojs:master Jan 18, 2017

misteroneill deleted the techs-not-components branch January 19, 2017 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make registerComponent only work with Components #3802

Make registerComponent only work with Components #3802

misteroneill commented Nov 26, 2016

brandonocasey Nov 28, 2016

misteroneill Nov 29, 2016

brandonocasey Nov 28, 2016

misteroneill Nov 29, 2016 •

edited

Loading

brandonocasey Nov 28, 2016

brandonocasey Nov 28, 2016

misteroneill Nov 29, 2016

misteroneill Dec 2, 2016

misteroneill Dec 2, 2016

brandonocasey Dec 2, 2016

misteroneill Dec 2, 2016

misteroneill Dec 20, 2016

gkatsev Dec 29, 2016

gkatsev Jan 4, 2017

misteroneill Jan 5, 2017

brandonocasey Jan 6, 2017 •

edited

Loading

misteroneill Jan 6, 2017

brandonocasey Jan 6, 2017

Make registerComponent only work with Components #3802

Make registerComponent only work with Components #3802

Conversation

misteroneill commented Nov 26, 2016

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill Nov 29, 2016 •

edited

Loading

brandonocasey Jan 6, 2017 •

edited

Loading