API for adding additonal players to ReactPlayer in runtime #364
Conversation
src/ReactPlayer.js
Outdated
| @@ -19,6 +27,11 @@ export default class ReactPlayer extends Component { | |||
| return true | |||
| } | |||
| } | |||
| for (let Player of additionalPlayers) { | |||
There was a problem hiding this comment.
How about we put this loop before the default players? This would let us override the default players via our component's canPlay if we want to.
src/ReactPlayer.js
Outdated
| @@ -56,6 +69,11 @@ export default class ReactPlayer extends Component { | |||
| return Player | |||
| } | |||
| } | |||
| for (let Player of additionalPlayers) { | |||
There was a problem hiding this comment.
Same as above (though I guess this is the one that actually matters)
There was a problem hiding this comment.
@andybarron yeah, we can, but i think we should as @cookpete about his opinion.
There was a problem hiding this comment.
I agree that these new loops should go above the regular players.
You could also do:
for (let Player of [ ...additionalPlayers, ...players ]) {for the single loop.
|
My only thought on this is that passing |
README.md
Outdated
| ReactPlayer.clearAdditionalPlayers(); | ||
| ``` | ||
|
|
||
| All responsibilities for keeping your player's API compatible with ReactPlayer is on you. |
There was a problem hiding this comment.
Not sure this line is necessary? Seems a bit aggressive.
There was a problem hiding this comment.
@cookpete i just wanted to protect you from issues related to changes in interfaces of your "internal" players.
There was a problem hiding this comment.
Ok so how about:
It is your responsibility to ensure that custom players keep up with any internal changes to ReactPlayer in later versions.
README.md
Outdated
| @@ -234,6 +234,23 @@ If you aren’t using React, you can still render a player using the standalone | |||
|
|
|||
| See [`jsFiddle` example](https://jsfiddle.net/krkcvx9s/) | |||
|
|
|||
| #### Adding your own players | |||
|
|
|||
| If you have your own player, that compatible with ReactPlayer's internal architecture, you can use it like this: | |||
There was a problem hiding this comment.
If you have your own player, that is compatible...
|
@cookpete about passing players as props, i'm not sure about it, since with this additional players we kinda make configuration for ReactPlayer as module. If we passing it with props, then we kinda mixing properties of different abstraction layers. And if you passing new props, you expecting changes. And what changes should be when you pass new set of players? Should we run whole process of getting and rendering proper player for passed url? Don't really know what is the best way, so i took simple way with static method and "configuration" approach |
Yeah this is fair enough. |
README.md
Outdated
|
|
||
| ```javascript | ||
| import YourOwnPlayer from './somewhere'; | ||
| ReactPlayer.addAdditionalPlayer(YourOwnPlayer); |
There was a problem hiding this comment.
This seems like a longer method name than necessary. Could it just be ReactPlayer.addPlayer()?
README.md
Outdated
| Or you can clear all additional players: | ||
|
|
||
| ```javascript | ||
| ReactPlayer.clearAdditionalPlayers(); |
There was a problem hiding this comment.
Slightly anal, but I think I prefer referring to the extra players as "custom players" rather than "additional players" throughout the codebase.
There was a problem hiding this comment.
@cookpete so then ReactPlayer.addCustomPlayer() and ReactPlayer.clearCustomPlayers()?
There was a problem hiding this comment.
addCustomPlayer and removeCustomPlayers would be better I think 👍
|
@cookpete i updated PR |
|
Nice one! Thanks @BrooklynKing and @andybarron. |
|
Published in |
|
@BrooklynKing @andybarron could either of you share an example of successfully using a custom player with this API? |
|
@shawninder not sure i can share example, since it's in private repo, but i can try to answer on your questions |
|
@BrooklynKing I'm getting weird errors I can't figure out with my custom player, yet it's basically copy-pasted from the FilePlayer... Here's a question. Did you also use If you're curious, here is my work-in-progress: https://github.com/shawninder/music/blob/master/components/IndexedDBPlayer.js It currently doesn't do anything (I was hoping to stop the Component from crashing the page before starting to add functionality) as it's just the FilePlayer copied, renamed, and with the File stuff removed. I just don't understand why this crashes the page whereas the FilePlayer included in react-player doesn't. |
|
@shawninder are you doing import { IndexedDBPlayer } from '...'or import IndexedDBPlayer from '...'as the latter would cause issues. |
|
Ah, damn it, I am using |
|
@cookpete While you're in the area, here's a question for you: If I do get this IndexedDBPlayer working, would you be interested in a pull request adding the player to this repo? I'm guessing not, since playing media from IndexedDB feels like an infrequent use-case, but if you do want it, just tell me and I'll submit a pull request when I'm "done". |
#362