Making some changes

[Aryk]

Hi @souporserious,

Thanks for writing this library! As I've been DEV-ing with it, I've ran into some challenges with it here and there and decided to make some changes:

• I also have SSR so I ran into the document not defined issues as well, which I solved for the use cases I was using. I believe the Audiocontext problem described in the ticket is still around though.
• I added startTime and endTime support for all media playing.
• Seek before the video was playing causes some weird behavior in Youtube, so I was trying to fix that.
• There were some issues in your example that were redundant or not needed (I think) so I cleaned those out.

There are more comments in the diff. I'm not doing a PR request because I don't think my changes are battle tested, but I wanted you to keep you in the loop.

[Aryk]

I was curious as to two things,

1. Why did you chose to use context as opposed to passing state down from Media to the Player component?
2. Why is the lifecycle, that the user calls a javascript function off of props.media and then the state changes as a result. From what I understand, the "React way" would be to change the state and then have the view change as a result. I guess it it's harder to reason about the library because there are two directions of state changing.

[souporserious]

Thanks for your feedback :) I appreciate it. I haven't been able to devote as much time as I'd like to this lately. Feel free to create a PR and we can start going through it together and making sure nothing is breaking if you would like. Everything looks good from what I can see though.

To answer your questions:

1. The reason for context is to allow library users to compose the component however they want. This way no matter where the Player component lives, it will be able to provide the Media component with the proper state to pass back down.

2. This was the easiest way I could think to allow people to build custom media components. I'm definitely open to a better React-y way of doing this if you have any ideas.

[Aryk]

I'm going to rewrite your library to address these two issues. In short, I will:

1. remove the need for context and this "magic"
2. remove the dependency on the Media container,
3. allow you to use any kind of state management you want. So if you save all your state in redux, you can use that with this library. :)
4. do all this while keeping the lower level components "dumb" without state of their own.

I'm going for it, let's see what i can do...

[souporserious]

Awesome :) excited to see what you can do! I can't promise it will be merged into here, but if we're able to build the same examples with what your setup and everything looks ok, I'll gladly merge it in. Please let me know if you have any questions about anything, I'll try and help as best as I can.

[Aryk]

Basically the model I'm trying to go for is.

action creator/function => state change => change player.

And then you would be able to use the method to pass the props down if you wanted to, or listen for state changes using Redux. I mean this is the way the developer is building the rest of the app, so I figured thats how they should interopt with this library, whereas your library is doing the above in the opposite direction.

[souporserious]

Ah I think I'm getting more of what you're saying. So essentially someone can use whatever state management flavor they choose, in your case redux, in my case I could build a wrapper component using context to pass state around. Am I getting that right?

[Aryk]

Yes, that's the idea. Actually, I would like to do away with context entirely. Instead, you can have a higher level smart "container" and pass it down to dumb "components".

Here are the changes I made:

https://github.com/souporserious/react-media-player/compare/master...Aryk:development

I essentially rewrote half of it. I can now have one container that holds the state and then pass down whatever I need to into my child components.

The examples work and the readme is updated, so I would recommend to read the readme first:

https://github.com/Aryk/react-media-player/blob/development/README.md

Here is an example of having one main higher level component with the state for playlist and videoplayer. This could all easily be in redux if I wanted to.

[souporserious]

Thank you for this issue and your work, currently the library is going to stay the same, but I will revisit this once I have time and look to do a rewrite that allows the use of libraries like Redux.

[Aryk]

Ok, agreed, I feel the two libraries are substantially different now, probably doesn't make sense to merge. Maybe I will release a branch for users who seek out this specific functionality if I find some time, but there are still some loose ends to fix. Thanks for getting back to me!

[souporserious]

@Aryk I'm looking to start redesigning this so it's better for things like Redux. Wondering if I can get your opinion/help on how to make it better? Unfortunately, I've only worked with Redux a couple of times and that was a while ago. I started sketching a new possible API. Any thoughts on something like this?

const onPlayPause = (isPlaying) => dispatch(isPlaying)

<Media state={reduxStoreOrWhatever} callbacks={{ onPlayPause }}>
  <Player src={...}/>
  <PlayPause/>
</Media>

So my idea is you can still use everything pretty much the same and the Media component uses context to pass the callbacks down to the proper components. But, if you need to, you can provide your own state and callbacks to the Media component so it can come from something like a Redux or Alt store. Does that makes sense or even sound like it would work? haha

[Aryk]

Yeah, but it doesn't seem to be ideal to have both context and Redux. The point of using something like redux and a central store is to avoid the need for things like React's context (which I believe more and more are moving away from). My refactoring did away with context completely in favor of a 'mediaHelper' which is constructed from the state and is hooked into your application via the react-redux library. It completely did away with the need for context so that all the data in your app is following the redux data flow.

[souporserious]

I believe context is here to stay. I don't know that for sure, but I know a good amount of popular libraries like React Router and React Redux itself rely on context, so I think it should be ok 😃 I'm sure there will be changes in the future, but at least it's only on me and not the users of this library.

When this library was first made, I didn't rely on context and had the problem of hooking up every element by spreading props on it which felt weird to me. The context allows the elements to subscribe with a HoC and nothing more, which IMO keeps things nice and manageable.

I see the benefit being that you can use React Media Player out of the box without any dependencies. Or if you would like, you can hook up something like Redux or Alt to manage your state.