Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 39 additions & 10 deletions src/VideoPlayer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,40 +11,69 @@
import React, { forwardRef } from 'react';

// We use and extend Typescript types defined in the MUX player.

import type MuxPlayerElement from '@mux/mux-player';
import type { MuxPlayerProps } from '@mux/mux-player-react';

// React MUX player is made available in two flavours: eager and lazy loaded. We
// choose to use the lazy version to avoid loading the web component uselessly.
// MUX player lazy version loads internally the eager version using
// `React.lazy()`.

import MuxPlayer from '@mux/mux-player-react/lazy';

// The core of this component is the `useVideoPlayer` hook: it takes
// data from DatoCMS GraphQL API and returns props as expected by the
// `<MuxPlayer />` component.

import { useVideoPlayer } from '../useVideoPlayer/index.js';

type Maybe<T> = T | null;
type Possibly<T> = Maybe<T> | undefined;

/**
* The playback ID is how the player knows which video to play.
* At least one of these is required.
* If both are provided, `muxPlaybackId` takes precedence.
* This is for backward compatibility.
*/
type PlaybackIdSources =
| { muxPlaybackId: Possibly<string> }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly means it can be null/undefined, so I think we have achieved nothing here 😆

Didn't we want to make one of them required?

Also, you've missed the JSDoc for the two properties.

| { playbackId: Possibly<string> };

// `Video` represents a fragment of data regarding a video as returned from
// DatoCMS GraphQL API.

export type Video = {
/**
* Video data as returned by the DatoCMS GraphQL Content Delivery API.
*
* You MUST provide `muxPlaybackId` (which is a field in your GraphQL query). The `streamingUrl` property is not used here.
Copy link
Member

@stefanoverna stefanoverna Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's streamingUrl?!

(I mean, I know what it is, but we should better explain it in here, or it will be even more confusing, since nowhere in this code or in the docs there's any mention of this property)

*
* As long as you provide `muxPlaybackId`, you can safely ignore the `playbackId` param. If you accidentally provide both, `muxPlaybackId` takes precedence.
*
* @property muxPlaybackId - Mux playback ID
* @property playbackId - (Optional, fallback only) Alias for the `muxPlaybackId` param, for backward compatibility
Copy link
Member

@stefanoverna stefanoverna Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

playbackId is not for backwards compatibility, it's just the original name of the prop for the underlying component. Silvano chose to support either our property name... or theirs. Which is not a good idea, since the use case for our <VideoPlayer /> is to just pass data={...} with the result of a GraphQL query to DatoCMS, not to manually pass single props. So why would anyone ever use data.playbackId?

So far, playbackId was kind of a hidden (and useless) feature, but now that we need to make muxPlaybackId, it became a PITA.

We should drop support for playbackId in the next major version, yes. For now we should silently support playbackId, deprecate it, and never mention it anywhere, so I'd remove the @property here.

* @property title - Title attribute (`title`) for the video.
* @property height - The height of the video in pixels.
* @property width - The width of the video in pixels.
* @property blurUpThumb - A `data:` URI containing a blurhash placeholder image.
* @property [k: string] - Any additional properties are accepted but not used
* by the renderer.
*
* @example
* ```ts
* const video: Video = {
* muxPlaybackId: "abcdef123456",
* title: "Example video",
* width: 1920,
* height: 1080,
* blurUpThumb: "data:image/png;base64,..."
* };
* ```
*/
export type Video = PlaybackIdSources & {
/** Title attribute (`title`) for the video */
title?: Possibly<string>;
/** The height of the video */
height?: Possibly<number>;
/** The width of the video */
width?: Possibly<number>;
/** The MUX playbaack ID */
muxPlaybackId?: Possibly<string>;
/** The MUX playbaack ID */
playbackId?: Possibly<string>;
/** A data: URI containing a blurhash for the video */
blurUpThumb?: Possibly<string>;
/** Other data can be passed, but they have no effect on rendering the player */
Expand All @@ -70,7 +99,7 @@ export const VideoPlayer: (
VideoPlayerProps
>((props, ref) => {
const {
data = {},
data = { muxPlaybackId: undefined },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this for? 😕

disableCookies = true,
disableTracking = true,
preload = 'metadata',
Expand Down