Conversation
cb8489c to
c356fa1
Compare
|
I switched the implementation to videojs instead of shaka because it was a pain to switch between html5 video for mp4/basic videos & shaka for hls/dash. |
|
I added new public APIs that are only used for the web right now:
|
I agree, but since we don’t currently have them on native side and Video.js v10 doesn’t provide a public API for this, we can add it in the future, for example when video.js will add support |
|
I'll personally wait for those features to be available to update and keep kyoo on my branch in the meantime. I skimmed through videojs's issues/milestone and it probably won't be long before they implement those features. They also have some nice features planned like keyboard control, chromecast/airplay... |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 47 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
packages/react-native-video/src/core/events/VideoPlayerEventsBase.ts:101
onBandwidthUpdateis part ofVideoPlayerEvents(no@platformrestriction) butVideoPlayerEventsBase.addEventListenerdoesn’t handle it anymore, so on web it will currently throwUnsupported event: onBandwidthUpdate. Either route this event here (like other shared events) viaeventEmitter.addOnBandwidthUpdateListener, or explicitly mark it as native-only inEvents.tsand ensure web doesn’t advertise support.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/react-native-video/src/core/video-view/VideoView.web.tsx
Outdated
Show resolved
Hide resolved
...s/react-native-video/android/src/main/java/com/twg/video/core/utils/VideoInformationUtils.kt
Outdated
Show resolved
Hide resolved
packages/react-native-video/src/core/video-view/VideoView.web.tsx
Outdated
Show resolved
Hide resolved
| "dependencies": { | ||
| "@videojs/react": "^10.0.0-beta.11" | ||
| }, |
There was a problem hiding this comment.
@videojs/react is added as a dependency instead of peerDependency to simplify DX for web users - no extra install needed. This package is only imported from .web.tsx files, which are never resolved by Metro when bundling for native platforms, so it has zero impact on native bundle size
Summary
Initial implementation for the web. I decided to use video.js to support more advanced features (like hls/dash/ads/drm...)
Missing features i don't plan on implementing:
Motivation
Fixes #4605
Note that i found some missing APIs while implementing this (crossed items are those I added an api & implementation but only on the web):
missingmimeTypein sourcestartTimemissing audio/video selectionsmulti audiosmulti variants (hls/dash only)Test plan
I've been using this PR for a few months on https://github.com/zoriya/kyoo and it works perfectly. This should be ready to merge.