feat(packages): add youtube media with html and react components - #1853
feat(packages): add youtube media with html and react components#1853luwes wants to merge 6 commits into
Conversation
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (11)
Players (5)
Skins (30)
UI Components (39)
Sizes are marginal over the root entry point. ⚛️ @videojs/react
Presets (7)
Media (10)
Skins (27)
UI Components (33)
Sizes are marginal over the root entry point. 🧩 @videojs/core
Entries (69)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (11)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretJS sizes are initial static graph totals (minified + brotli). Lazy dynamic chunks are shown separately when present.
Run |
| // Unblock callers awaiting load so play()/fullscreen don't hang. | ||
| this.#loadComplete.resolve(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Stale attach error unblocks load
Medium Severity
The #createPlayer failure path sets #error, emits error, and resolves #loadComplete without checking #attachId / #target. A failed API load from a detached attach can mark a newer attach as failed and unblock its play() before the new player is ready.
Reviewed by Cursor Bugbot for commit 23a842a. Configure here.
| frameborder="0" | ||
| width="100%" | ||
| height="100%" | ||
| referrerpolicy="${escapeHtml(attrs.referrerpolicy ?? '')}" |
There was a problem hiding this comment.
Missing YouTube referrerPolicy default
Medium Severity
The HTML and React YouTube iframes omit a usable default referrerpolicy. On pages with strict Referrer-Policy (or other no-referrer contexts), YouTube serves Error 153 (embedder.identity.missing.referrer) and the embed fails unless callers set config.referrerPolicy themselves.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 23a842a. Configure here.
| this.#player = player; | ||
| this.#bindPlayerEvents(player); | ||
| this.#setupTextTracks(player); | ||
| } |
There was a problem hiding this comment.
Detach race leaves load unguarded
High Severity
#createPlayer checks #attachId only on the success path after loadYouTubeApi. The catch path, and the onReady / onError callbacks, still mutate state after detach() (or a newer attach()). That can mark a fresh attach as loaded, resolve its #loadComplete early, restart polling, or set #playerReady before the new player is ready so a later cueVideoById runs too soon.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ba0d777. Configure here.
| const EMBED_BASE_NOCOOKIE = 'https://www.youtube-nocookie.com/embed'; | ||
| const VIDEO_MATCH_SRC = | ||
| /(?:youtu\.be\/|youtube(?:-nocookie)?\.com\/(?:embed\/|v\/|watch\?v=|watch\?.+&v=|shorts\/|live\/))((?:\w|-){11})/; | ||
| const PLAYLIST_MATCH_SRC = /(?:youtu\.be\/|youtube(?:-nocookie)?\.com\/.*?[?&]list=)([\w-]+)/; |
There was a problem hiding this comment.
Playlist regex misfires on youtu.be
Medium Severity
PLAYLIST_MATCH_SRC treats the youtu.be/ alternative as enough to capture the next path segment as listId, without requiring a list= param. Every short link therefore gets a spurious listId (usually the video id), and non-11-character youtu.be/... paths are misclassified as kind: 'playlist' instead of failing to parse.
| const PLAYLIST_MATCH_SRC = /(?:youtu\.be\/|youtube(?:-nocookie)?\.com\/.*?[?&]list=)([\w-]+)/; | |
| const PLAYLIST_MATCH_SRC = /(?:youtu\.be\/|youtube(?:-nocookie)?\.com\/).*?[?&]list=([\w-]+)/; |
Reviewed by Cursor Bugbot for commit ba0d777. Configure here.
| async requestFullscreen() { | ||
| await this.#target?.requestFullscreen?.(); | ||
| this.#isFullscreen = true; | ||
| } |
There was a problem hiding this comment.
Fullscreen flag set without success
Low Severity
requestFullscreen always sets #isFullscreen to true after awaiting the optional call. If #target is null or requestFullscreen is missing, nothing enters fullscreen but the host still reports that it did, so UI that trusts isFullscreen can get stuck in the wrong state.
| } | |
| async requestFullscreen() { | |
| if (!this.#target?.requestFullscreen) return; | |
| await this.#target.requestFullscreen(); | |
| this.#isFullscreen = true; | |
| } |
Reviewed by Cursor Bugbot for commit ba0d777. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 7 total unresolved issues (including 6 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e523d13. Configure here.
| this.#player = player; | ||
| this.#bindPlayerEvents(player); | ||
| this.#setupTextTracks(player); | ||
| } |
There was a problem hiding this comment.
Stale ready callbacks after detach
High Severity
#attachId only guards the pre-Player creation path. onReady, onError, and bound state listeners never check it, so a callback from a destroyed player can still run after detach() / re-attach() (common with React Strict Mode). That can flip #loaded / #playerReady, resolve the new session’s #loadComplete early, start a leaked poll timer, and make the real onReady no-op so play() never proceeds.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e523d13. Configure here.
| 'dom/media/simple-hls-audio-only/index': './src/dom/media/simple-hls-audio-only/index.ts', | ||
| 'dom/media/simple-hls/index': './src/dom/media/simple-hls/index.ts', | ||
| 'dom/media/vimeo/index': './src/dom/media/vimeo/index.ts', | ||
| 'dom/media/youtube/index': './src/dom/media/youtube/index.ts', |
There was a problem hiding this comment.
super nit but it might be nicer if these files were name differently so it's a little easier to discover them, rather than index.ts, which strikes me as a barrel file.
There was a problem hiding this comment.
Also, a wider change rather than related directly to this PR...
2 participants
Refs #1434
Summary
Adds a YouTube media host so the player can drive YouTube embeds the same way it drives Vimeo ones. Ports the playback logic from
youtube-video-elementinto the v10 media-host architecture, mirroring the existing Vimeo media structure across core, html, and react.Changes
YouTubeMediacore host: attach/detach lifecycle, state mapped from iframe API events, played-range tracking, caption tracklist syncing, and loop-on-ended (the API has no single-video loop)youtu.be/ embed / shorts / live URLs, raw 11-char ids, playlists (list=),t=2m51sstart times, and the-nocookiehost<youtube-video>custom element (@videojs/html/media/youtube-video) and<YouTubeVideo>React component (@videojs/react/media/youtube-video)Implementation details
loadScriptutility with minimal local typings — no new dependencytimeupdate/progress/seekingevents, so a 50ms poll derives them (same approach as the original element)srcchanges reuse the player viacueVideoById/loadVideoById(or the playlist variants) instead of rebuilding the iframe; any post-load state transition completes the load sinceonReadyonly fires onceTesting
pnpm -F @videojs/core test src/dom/media/youtube— 32 unit tests covering URL parsing, embed URL building, andYouTubeMediabehavior against a mockedYT.Player. Manual:pnpm dev→ sandbox → “YouTube Video” preset (HTML and React).Note
Medium Risk
Large new async embed integration with external script loading and lifecycle edge cases, but it mirrors proven Vimeo patterns and has broad unit test coverage.
Overview
Adds YouTube embed playback to the v10 media-host stack (alongside Vimeo): a new
YouTubeMediacore adapter loads the iframe API, maps player state to standard media events, polls for time/progress updates, syncs captions, and handles playlists, start times, and nocookie URLs.Ships
<youtube-video>(HTML) and<YouTubeVideo>(React) wired through the same iframe attach pattern as Vimeo, plus a sandbox “YouTube Video” preset with HTML/React demo pages.Vimeo
detach()now resolves the load promise soplay()does not hang if the component unmounts before load completes (YouTube uses the same guard).Reviewed by Cursor Bugbot for commit 7e5803e. Bugbot is set up for automated code reviews on this repo. Configure here.