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.

Sign up for GitHub

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

Jump to bottom

Don't throw when re-registering a plugin #4140

Merged
merged 2 commits into from
Mar 2, 2017
Merged

Don't throw when re-registering a plugin #4140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1655f2b
Don't throw when re-registering a plugin
misteroneill Feb 28, 2017
10bb8c1
Alright, throw if it's a player method
misteroneill Feb 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/js/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import evented from './mixins/evented';
import stateful from './mixins/stateful';
import * as Events from './utils/events';
import * as Fn from './utils/fn';
import log from './utils/log';
import Player from './player';

/**
Expand Down Expand Up @@ -305,8 +306,10 @@ class Plugin {
throw new Error(`Illegal plugin name, "${name}", must be a string, was ${typeof name}.`);
}

if (pluginExists(name) || Player.prototype.hasOwnProperty(name)) {
throw new Error(`Illegal plugin name, "${name}", already exists.`);
if (pluginExists(name)) {
log.warn(`A plugin named "${name}" already exists. You may want to avoid re-registering plugins!`);
Copy link
Member

Choose a reason for hiding this comment

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

@misteroneill when you get the chance, can you make another PR that silences this warning in the plugin tests?

} else if (Player.prototype.hasOwnProperty(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.

This is an else if because if a plugin exists, it will have a method on the Player.prototype, but if the plugin does not exist and the name matches a Player.prototype method, it's an actual player method.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a reasonable compromise.

throw new Error(`Illegal plugin name, "${name}", cannot share a name with an existing player method!`);
}

if (typeof plugin !== 'function') {
Expand Down
20 changes: 14 additions & 6 deletions test/unit/plugin-static.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
/* eslint-env qunit */
import sinon from 'sinon';
import log from '../../src/js/utils/log';
import Player from '../../src/js/player';
import Plugin from '../../src/js/plugin';

Expand Down Expand Up @@ -53,12 +55,6 @@ QUnit.test('registerPlugin() illegal arguments', function(assert) {
'plugins must have a name'
);

assert.throws(
() => Plugin.registerPlugin('play'),
new Error('Illegal plugin name, "play", already exists.'),
'plugins cannot share a name with an existing player method'
);

assert.throws(
() => Plugin.registerPlugin('foo'),
new Error('Illegal plugin for "foo", must be a function, was undefined.'),
Expand All @@ -70,6 +66,18 @@ QUnit.test('registerPlugin() illegal arguments', function(assert) {
new Error('Illegal plugin for "foo", must be a function, was object.'),
'plugins must be functions'
);

assert.throws(
() => Plugin.registerPlugin('play', function() {}),
new Error('Illegal plugin name, "play", cannot share a name with an existing player method!'),
'plugins must be functions'
);

sinon.spy(log, 'warn');
Plugin.registerPlugin('foo', function() {});
Plugin.registerPlugin('foo', function() {});
assert.strictEqual(log.warn.callCount, 1, 'warn on re-registering a plugin');
log.warn.restore();
});

QUnit.test('getPlugin()', function(assert) {
Expand Down