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

Projects
None yet
2 participants
@misteroneill
Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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 from feat: Add `videojs.all` to get an array of players or delegate method calls to all players. to feat: Add `videojs.getAllPlayers` to get an array of players. Jan 5, 2018

@gkatsev

gkatsev approved these changes Jan 5, 2018

LGTM

@gkatsev

This comment has been minimized.

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@misteroneill misteroneill deleted the misteroneill:feat/all branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment