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

Restore options/players Globals #2395

Closed
wants to merge 7 commits into from

Conversation

misteroneill
Copy link
Member

Addresses #2311.

This restores the options and players properties of videojs.

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, modifying videojs.players will log a deprecation warning where internal modifications to Player.players will not.

Both cases - using Proxy and not - are tested.

* @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) => {
Copy link
Member

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.

Copy link
Member Author

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!

@misteroneill
Copy link
Member Author

Moved createDeprecationProxy into a module and changed its signature a bit (removing the name argument).

@gkatsev
Copy link
Member

gkatsev commented Jul 22, 2015

LGTM

@mmcc
Copy link
Member

mmcc commented Jul 22, 2015

@pam retry

1 similar comment
@mmcc
Copy link
Member

mmcc commented Jul 22, 2015

@pam retry

@heff
Copy link
Member

heff commented Jul 23, 2015

Nit-picky but I'd prefer the warning messages to have a capital first letter.

Looks good to me.

@misteroneill
Copy link
Member Author

@heff Sounds good (and done).

@dmlap dmlap closed this in a76b42d Jul 30, 2015
@misteroneill misteroneill deleted the restore-globals branch August 3, 2015 20:49
@heff heff mentioned this pull request Aug 20, 2015
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.

4 participants