-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
throw error when plugin is missing #1931
Conversation
@@ -99,7 +99,11 @@ vjs.Player = vjs.Component.extend({ | |||
|
|||
if (options['plugins']) { | |||
vjs.obj.each(options['plugins'], function(key, val){ | |||
this[key](val); | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than using try/catch, we can see whether this[key]
is a function and then log an error. This way it continues working even if a plugin is missing but we still have a better error log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to figure out what the right way to go was here but, in my case, my video.js player is just an empty shell without the plugin and I want to abort further loading. It sounds like logging it would not help me determine if the video.js player (and required plugin) is broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe log an error, fire an error event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could anyone help me figure out why this build is dying? |
It looks like it doesn't like |
|
and then you should use |
Ok, thanks I'll check it out asap. I used the window reference because I saw this being used in text track tests, doesn't seem any other tests that check for a throw. |
It doesn't fix my local jshint 'issues' though, but I guess that's a different PR. |
Adding
Since I'll just use |
Why are the tests minified anyway? This causes unexpected results where the key name is minified it seems:
I guess making it a string key in the test fixes it. |
Updated pull request and now a plugin that does not exist logs an error. |
We don't need |
Improve error for missing plugin, refs #1928.