benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Add media prop to MediaPlayer and PlayerContext for custom media element injection. #347

Closed iantbutler01 closed 5 years ago

iantbutler01 commented 5 years ago

Basically the use case for me is that I'm relying heavily on OGG format for audio because it compresses better then MP3 and will save me money on storage and transport for a large number of these files for an application I'm working on. The issue is Safari and IE 10 do not natively support OGG so enter https://github.com/brion/ogv.js/ as a battle tested polly fill.

I want to be able to inject their media element into MediaPlayer and then PlayerContext if I detect Safari or IE to achieve support for OGG.

Doesn't look to bad to enable here since it would just be a pass-through to the context, but there may be hidden gotchas so I'd like to discuss.

Let me know if you think this is a good feature and I'll open a PR when I finish.

benwiley4000 commented 5 years ago

On a high level I think this makes sense to me - you basically want to define your own HTMLMediaElement substitute? This would be awesome especially because I've been trying to think about ways to make a mocked version of the video element available for testing purposes and this could be a step toward that.

Here's what I'm picturing in terms of API. If that makes sense to you, you have my blessing to make a PR. Thanks!

<PlayerContextProvider {...otherProps} createMediaElement={() => new OGVPlayer()}>
  {children}
</PlayerContextProvider>

And that prop would be used here instead of document.createElement('video').

BTW 2 things:

  1. I checked out the docs for ogv.js and it says it supports a subset of the API/events for the HTMLVideoElement, which might be fine, but I just wanted to make sure it supports all the Cassette features you want to use! :) In particular I'm wondering if it will support the mutation observer and src setter we use to make sure outside manipulation of the media element is synced properly with cassette's internal state. These are mostly to protect against niche use cases though, which you probably wouldn't run into. It might work! I just have no idea how the internals of this library work.
  2. IE 10 probably doesn't work at the moment (I haven't tested it, but we use MutationObserver which appears to have been introduced in IE11). We can fix this by using mutation events as a fallback and that should extend support to IE9 (the ResizeObserver polyfill we use does this to support IE9). But again I haven't tested at all in IE so I'm not sure what our browser support range is at the moment. We have an issue for this, and if you want to help out, I'd be happy to let you. :slightly_smiling_face: We have authorization to use BrowserStack for cross-browser testing, for free, if that's helpful.
benwiley4000 commented 5 years ago

Updated my previous comment with some links.

iantbutler01 commented 5 years ago

Yup exactly, the only other thought I would add is also allowing this to be injected from the high level MediaPlayer and passed into the PlayerContextProvider if it is passed as a prop there. Just to make it easy to keep using that high-level component.

To your points:

  1. I just checked out ogv again and can confirm they support src setter. However, the mutation observer looks like something I will have to do a trial and error approach on though.

  2. That's fine, when I finish this one I'll poke around a bit and see how far back it works till, one thing to consider not sure how you look at external dependencies but, https://github.com/megawac/MutationObserver.js?files=1 is apparently a thing and your use case seems pretty normal so that pollyfill covers IE back to 8 could be as simple as bundling Cassette with that for a solution instead of having to build out the media event flow.

If that sounds good I will get something up this week since ogg support means a lot to me.

benwiley4000 commented 5 years ago

Any prop sent to MediaPlayer that isn't for MediaPlayerControls gets sent to PlayerContextProvider automatically - so you shouldn't have to worry about touching that file (I think).

As for the MutationObserver polyfill.. that's an interesting idea! My main reservation about adding it to the bundle is that adds 24kb to the unminified file size to support very old browsers. I'd rather suggest that users bring their own polyfill (or there could be an official polyfills package). The reason I'd like to look at the mutation events fallback is it could be much cheaper to implement that for the specific use case, although I haven't done much research yet. Either way I think that's outside the scope of this change.. I'll create a new issue for that.

Back to the point .. thanks for working on this! I look forward to seeing the PR. :)

benwiley4000 commented 5 years ago

Here's the issue I made about supporting IE9: https://github.com/benwiley4000/cassette/issues/348

benwiley4000 commented 5 years ago

Closed in #349.