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

feat: Add videojs.getAllPlayers to get an array of players. #4842

Merged
merged 2 commits into from
Jan 30, 2018

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented Dec 24, 2017

When working on another PR, I noticed that there were tests somewhere not completely cleaning up after themselves. Then, I thought, wouldn't it be nice to be able to delegate method calls to all players, so we could dispose them?

This new function makes that easier:

videojs.getAllPlayers().forEach(p => p.dispose());

Previously, you'd have to do something like:

Object.keys(videojs.players).map(k => videojs.players[k]).filter(Boolean).forEach(p => p.dispose());

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2018

I'm for adding these but against this API. Having it return an array of players when called via .all() and then call the players when called via .all('dispose') is confusing and inconsistent.
What about keeping the calling functionality for calling methods and moving the returning the array to a .getAllPlayers() method? Also, potentially move the calling to something like .playersDo(), which has precedent in vim and similar functionality like :tabdo and :bufdo where you do something for each tab.

@misteroneill
Copy link
Member Author

misteroneill commented Jan 2, 2018

Yeah, I don't disagree. I had similar thoughts.

I like getAllPlayers for getting an array, but playersDo seems confusing to me. Maybe callAllPlayers or delegateAllPlayers?

Alternatively, the callAllPlayers could be ignored entirely and you could simply do:

videojs.getAllPlayers().forEach(p => p.dispose());

Another alternative approach would be to have getAllPlayers return an array-like object with Player prototype methods mixed in:

const playerCollection = videojs.getAllPlayers();

playerCollection.dispose();

const playerArray = videojs.getAllPlayers().toArray();

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2018

I'm partial to callAllPlayers but yeah, maybe leave it out for now until we have a real use-case or see people wanting to use it more?

@misteroneill misteroneill changed the title feat: Add videojs.all to get an array of players or delegate method calls to all players. feat: Add videojs.getAllPlayers to get an array of players. Jan 5, 2018
@misteroneill misteroneill force-pushed the feat/all branch 2 times, most recently from e84c103 to 818684e Compare January 5, 2018 17:49
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

LGTM

@gkatsev gkatsev added confirmed minor This PR can be added to a minor release. It should not be added to a patch release. labels Jan 5, 2018
@gkatsev
Copy link
Member

gkatsev commented Jan 23, 2018

Tested: ☑️

@gkatsev gkatsev added the tested label Jan 25, 2018
@gkatsev gkatsev merged commit 6a00577 into videojs:master Jan 30, 2018
@misteroneill misteroneill deleted the feat/all branch January 30, 2018 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release. tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants