Make the Mux Playback ID more explicit #115
Conversation
✅ Deploy Preview for react-datocms-example canceled.
|
stefanoverna
left a comment
stefanoverna
left a comment
There was a problem hiding this comment.
There are a few things to fix, also, we should release this when we have similar PRs ready for all the other <VidePlayer /> we offer (svelte, vue, etc.)
| * This is for backward compatibility. | ||
| */ | ||
| type PlaybackIdSources = | ||
| | { muxPlaybackId: Possibly<string> } |
There was a problem hiding this comment.
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.
| * 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 |
There was a problem hiding this comment.
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.
| /** | ||
| * 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. |
There was a problem hiding this comment.
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)
| >((props, ref) => { | ||
| const { | ||
| data = {}, | ||
| data = { muxPlaybackId: undefined }, |
2 participants
Fix for #113
muxPlaybackIdorplaybackIdis required