-
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
Restore options/players Globals #2395
Conversation
* @param {Object} messages String messages to display from a Proxy, mapping to get/set operations. | ||
* @return {Object} A Proxy if supported or the `target` argument. | ||
*/ | ||
const createDeprecationProxy = (name, target, messages) => { |
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.
can we have this return either the proxy or just the target?
Then below in your usage we could do
videojs.players = createDeprecationProxy(Players.players, {
get: 'access to videojs.players is deprecated; use videojs.getPlayers instead',
set: 'modification of videojs.players is deprecated'
});
Also, we should consider putting this function into its own file in src/utils/
so that others could possibly use it as well.
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.
👍 on both.
The name
argument is a holdover from the original implementation, which generated the log messages. Since that doesn't happen anymore, it's superfluous and your suggestion is better!
4f62c42
to
02523a4
Compare
Moved |
LGTM |
@pam retry |
1 similar comment
@pam retry |
Nit-picky but I'd prefer the warning messages to have a capital first letter. Looks good to me. |
16d92d4
to
84cb3a1
Compare
@heff Sounds good (and done). |
Also, add a test for the set case.
84cb3a1
to
0465c04
Compare
Addresses #2311.
This restores the
options
andplayers
properties ofvideojs
.Browsers with
Proxy
support (Firefox and IE Edge at the moment) will additionally log a warning on both get/set operations only through these specific properties. That is, modifyingvideojs.players
will log a deprecation warning where internal modifications toPlayer.players
will not.Both cases - using
Proxy
and not - are tested.