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

Tech registry #2782

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,10 @@ class Component {
// Add a direct reference to the child by name on the parent instance.
// If two of the same component are used, different names should be supplied
// for each
this[name] = this.addChild(name, opts);
let newChild = this.addChild(name, opts);
if (newChild) {
this[name] = newChild;
}
};

// Allow for an array of children details to passed in the options
Expand Down
7 changes: 6 additions & 1 deletion src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import TextTrackSettings from './tracks/text-track-settings.js';
import ModalDialog from './modal-dialog';

// Require html5 tech, at least for disposing the original video tag
import Tech from './tech/tech.js';
import Html5 from './tech/html5.js';

/**
Expand Down Expand Up @@ -1653,7 +1654,11 @@ class Player extends Component {
// Loop through each playback technology in the options order
for (let i = 0, j = this.options_.techOrder; i < j.length; i++) {
let techName = toTitleCase(j[i]);
let tech = Component.getComponent(techName);
let tech = Tech.getTech(techName);

if (!tech) {
tech = Component.getComponent(techName);
Copy link
Member

Choose a reason for hiding this comment

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

This would mean that the tech was only registered as a component? I think it's worth a comment above this to remind ourselves to one day get rid of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this is basically to support the now-deprecated feature of registering techs as components.

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'll add a comment.

}

// Check if the current tech is defined before continuing
if (!tech) {
Expand Down
1 change: 1 addition & 0 deletions src/js/tech/flash.js
Original file line number Diff line number Diff line change
Expand Up @@ -548,4 +548,5 @@ Flash.getEmbedCode = function(swf, flashVars, params, attributes){
FlashRtmpDecorator(Flash);

Component.registerComponent('Flash', Flash);
Tech.registerTech('Flash', Flash);
export default Flash;
1 change: 1 addition & 0 deletions src/js/tech/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -1087,4 +1087,5 @@ Html5.disposeMediaElement = function(el){
};

Component.registerComponent('Html5', Html5);
Tech.registerTech('Html5', Html5);
export default Html5;
41 changes: 41 additions & 0 deletions src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,46 @@ class Tech extends Component {
component instanceof Tech ||
component === Tech;
}

/**
* Registers a Tech
*
* @param {String} name Name of the Tech to register
* @param {Object} tech The tech to register
* @static
* @method registerComponent
*/
static registerTech(name, tech) {
if (!Tech.techs_) {
Tech.techs_ = {};
}

if (!Tech.isTech(tech)) {
throw new Error(`Tech ${name} must be a Tech`);
}

Tech.techs_[name] = tech;
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 this should check Tech.isTech(tech) and - possibly - throw if not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I'm thinking about it, should we do that?
Is a Tech specifically something that inherits from Tech or can it be anything that follows the same API?

Copy link
Member

Choose a reason for hiding this comment

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

Good question.

In the case that we don't necessarily want to inherit from Tech, we could do some kind of duck typing to enforce a tech's interface. Of course, that begs the question, do we want to go down that road or just leave it as-is.

return tech;
}

/**
* Gets a component by name
*
* @param {String} name Name of the component to get
* @return {Component}
* @static
* @method getComponent
*/
static getTech(name) {
if (Tech.techs_ && Tech.techs_[name]) {
return Tech.techs_[name];
}

if (window && window.videojs && window.videojs[name]) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there should probably be a Tech.isTech(window.videojs[name]) condition here to prevent misleading log.warn calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

What should it do if it isn't a tech? Component doesn't do that checking currently either and I basically just copied the code from there.
I think it makes sense to warn/throw if you're trying to register something as a tech that isn't, this function could be slightly more permissive.

Copy link
Member

Choose a reason for hiding this comment

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

Not 100% sure on that - or that anything needs to be done, given more reflection.

My original thinking was that if someone were to reach this with a value that wasn't a tech, but was available on the window, it would suggest they were doing something really weird and we shouldn't say "don't access your techs like this". Perhaps the log.warn is enough to suggest they did something strange/wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided not to do anything here but throw if trying to register a non-tech as a tech.

Copy link
Member

Choose a reason for hiding this comment

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

👍

log.warn(`The ${name} tech was added to the videojs object when it should be registered using videojs.registerTech(name, tech)`);
return window.videojs[name];
}
}
}

/*
Expand Down Expand Up @@ -649,4 +689,5 @@ Tech.withSourceHandlers = function(_Tech){
Component.registerComponent('Tech', Tech);
// Old name for Tech
Component.registerComponent('MediaTechController', Tech);
Tech.registerTech('Tech', Tech);
export default Tech;
46 changes: 45 additions & 1 deletion src/js/video.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import createDeprecationProxy from './utils/create-deprecation-proxy.js';
import xhr from 'xhr';

// Include the built-in techs
import Tech from './tech/tech.js';
import Html5 from './tech/html5.js';
import Flash from './tech/flash.js';

Expand Down Expand Up @@ -202,7 +203,50 @@ videojs.getComponent = Component.getComponent;
* @mixes videojs
* @method registerComponent
*/
videojs.registerComponent = Component.registerComponent;
videojs.registerComponent = (name, comp) => {
if (Tech.isTech(comp)) {
log.warn(`The ${name} tech was registered as a component. It should instead be registered using videojs.registerTech(name, tech)`);
}

Component.registerComponent.call(Component, name, comp);
};

/**
* Get a Tech class object by name
* ```js
* var Html5 = videojs.getTech('Html5');
* // Create a new instance of the component
* var html5 = new Html5(options);
* ```
*
* @return {Tech} Tech identified by name
* @mixes videojs
* @method getComponent
*/
videojs.getTech = Tech.getTech;

/**
* Register a Tech so it can referred to by name.
* This is used in the tech order for the player.
*
* ```js
* // get the Html5 Tech
* var Html5 = videojs.getTech('Html5');
* var MyTech = videojs.extend(Html5, {});
* // Register the new Tech
* VjsButton.registerTech('Tech', MyTech);
* var player = videojs('myplayer', {
* techOrder: ['myTech', 'html5']
* });
* ```
*
* @param {String} The class name of the tech
* @param {Tech} The tech class
* @return {Tech} The newly registered Tech
* @mixes videojs
* @method registerTech
*/
videojs.registerTech = Component.registerTech;

/**
* A suite of browser and device tests
Expand Down