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

fix: copy basic plugin properties onto the wrapper #4100

Merged
merged 2 commits into from
Feb 21, 2017

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Feb 17, 2017

Description

Copy basic plugin properties to prevent breaking older basic plugins

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
  • Reviewed by Two Core Contributors

src/js/plugin.js Outdated
@@ -312,6 +312,9 @@ class Plugin {
if (name !== BASE_PLUGIN_NAME) {
if (Plugin.isBasic(plugin)) {
Player.prototype[name] = createBasicPlugin(name, plugin);
Object.keys(plugin).forEach(function(prop) {
Player.prototype[name][prop] = plugin[prop];
});
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 this belongs in the createBasicPlugin function.

Copy link
Contributor Author

@brandonocasey brandonocasey Feb 17, 2017

Choose a reason for hiding this comment

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

I thought that too at first, but Player.prototype[name] does not exist until we get the return value from createBasicPlugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we really want it in createBasicPlugin, then we have to do the Player.prototype[name] assign in there as well. I don't see any problems with that. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

couldn't it just copy the props from plugin onto the instance that createBasicPlugin returns`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basic plugins don't have to return an instance though

Copy link
Member

Choose a reason for hiding this comment

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

createBasicPlugin is a function that returns a function. It's a bit confusing.
But instance is inside the returned function, you'd want to do the property copying in the outer arrow function instead.

Copy link
Member

Choose a reason for hiding this comment

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

I almost thought just now that createBasicPlugin initialized basic plugins when called, but it doesn't do that.

Copy link
Contributor Author

@brandonocasey brandonocasey Feb 17, 2017

Choose a reason for hiding this comment

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

yeah calling the function that createBasicPlugin returns on player is actually what initializes. The question is do we want to copy the methods that are on the plugin right away (this is how it used to work in 5.x)? or only after the plugin has been initialized.

Examples:

After initialization copy properties

var basic = () => {};
basic.VERSION = '9.9.9';
videojs.registerPlugin('basic', basic);
var player = videojs('some-id');

console.log(player.basic.VERSION);
// undefined

player.basic();
console.log(player.basic.VERSION);
// 9.9.9

Before initialization copy properties

var basic = () => {};
basic.VERSION = '9.9.9';
videojs.registerPlugin('basic', basic);
var player = videojs('some-id');

console.log(player.basic.VERSION);
// '9.9.9'

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 want to mimic current behavior, which means that we want to copy it before init and the plugin would need to handle things after init, like they do now.

@brandonocasey brandonocasey force-pushed the fix/better-plugin-wrapper branch from 2616d80 to afa747c Compare February 17, 2017 18:58
@brandonocasey brandonocasey force-pushed the fix/better-plugin-wrapper branch from 3fa1de0 to 8caed38 Compare February 17, 2017 19:54
@gkatsev
Copy link
Member

gkatsev commented Feb 21, 2017

Tests are fine except that IE8 is being weird on browserstack.

@gkatsev gkatsev merged commit 127cd78 into master Feb 21, 2017
@gkatsev gkatsev deleted the fix/better-plugin-wrapper branch February 21, 2017 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants