Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make registerComponent only work with Components #3802

Merged
merged 4 commits into from
Jan 18, 2017

Conversation

misteroneill
Copy link
Member

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

@misteroneill misteroneill added the needs: discussion Needs a broader discussion label Nov 26, 2016
@misteroneill misteroneill changed the title registerComponent only works with Components Make registerComponent only work with Components Nov 28, 2016
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@misteroneill misteroneill Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new Error('illegal "Comp"; must be a subclass of Component');
}

name = toTitleCase(name.trim());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am removing the trim().

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];
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@misteroneill misteroneill removed the needs: discussion Needs a broader discussion label Dec 2, 2016
return;
static registerComponent(name, ComponentToRegister) {
if (typeof name !== 'string' || !name.length) {
throw new Error(`illegal component name, "${name}"; must be a non-empty string`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static registerComponent(name, comp) {
if (!name) {
return;
static registerComponent(name, ComponentToRegister) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (name === 'Player' && Component.components_[name]) {
const Player = Component.components_[name];
const players = Component.components_[name].players;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new Error(`Illegal component name, "${name}"; must be a non-empty string.`);
}

const Tech = Component.getComponent('Tech');
Copy link
Contributor

@brandonocasey brandonocasey Jan 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok makes sense

@brandonocasey brandonocasey added the needs: discussion Needs a broader discussion label Jan 6, 2017
@misteroneill misteroneill removed the needs: discussion Needs a broader discussion label Jan 6, 2017
@gkatsev gkatsev merged commit 57af15c into videojs:master Jan 18, 2017
@misteroneill misteroneill deleted the techs-not-components branch January 19, 2017 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants