-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(playback-core): error migration et al. #965
feat(playback-core): error migration et al. #965
Conversation
@cjpillsbury is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
7f39bbd
to
4126665
Compare
packages/playback-core/src/index.ts
Outdated
} = props; | ||
const [playbackId] = playbackIdWithOptionalParams ? toPlaybackIdParts(playbackIdWithOptionalParams) : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Need to account for the potential additional params in playback-core because of our <mux-video>
and <mux-audio>
implementation of playbackId
...
packages/playback-core/src/errors.ts
Outdated
@@ -17,6 +29,8 @@ export class MediaError extends Error { | |||
|
|||
name: string; | |||
code: number; | |||
public muxCode?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: plausibly want to drop errorCategory
and muxCode
, but keeping for now just in case, since error context, message, and code are information lossy.
}, | ||
}; | ||
}; | ||
|
||
export const getAppCertificate = async (appCertificateUrl: string) => { | ||
const resp = await fetch(appCertificateUrl); | ||
if (resp.status !== 200) { | ||
return Promise.reject(resp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: we need some way to error with enough info to introspect why the request failed. Currently simply rejecting with the non-200 Response
, but we can definitely consider alternatives. Since this function scope doesn't have sufficient information to fully validate information (e.g. a mismatched playbackId
+ JWT sub
), we can't do a full translation here without passing in additional context.
4126665
to
23f6f9f
Compare
@@ -63,6 +63,7 @@ | |||
"mux-embed": "~5.2.0" | |||
}, | |||
"devDependencies": { | |||
"@mux/esbuilder": "0.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Using (an updated) esbuilder for playback-core as well to account for language json files.
} as const; | ||
|
||
export const MuxErrorCode = { | ||
NOT_AN_ERROR: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: I implemented unique codes for unique errors for elements, which we can use at various "levels" to translate into additional info (namely, the error dialog for mux player). With the new architecture, we rely on the newly added error category (i.e. was it e.g. DRM or Video, defined above) + error code (i.e. was it a network error or an encrypted error) to provide human readable info (i.e. "Your drm-token was expired")
@@ -0,0 +1,255 @@ | |||
import type { LoaderResponse } from 'hls.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: We may want to clean up our organization of error modules. I'm definitely amenable to relocating this, consolidating, etc.
@@ -9,9 +9,30 @@ const camelCase = (name) => { | |||
}; | |||
|
|||
const args = process.argv.slice(3).reduce((processArgs, val) => { | |||
let [key, value] = val.split('='); | |||
let [key, value = true] = val.split('='); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Needed to update esbuilder to account for multiple, arg-provided --external
arguments.
@@ -56,8 +77,10 @@ const options = { | |||
bundle: true, | |||
target: 'es2019', | |||
minify: args.minify, | |||
sourcemap: args.sourcemap ?? false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to account for sourcemap usage in playback-core.
case MediaError.MEDIA_ERR_NETWORK: { | ||
dialog.title = i18n(`Network Error`, translate); | ||
dialog.message = error.message; | ||
const muxMediaErrorToDialogTitle = (mediaError: MediaError, translate = false) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: For the error dialog text, I broke each text section (i.e. title + message) into separate functions that translate from code + category into the corresponding text. If this feels a bit too unwieldy, we can refactor to just have a single lookup function for both, though this does mean we'll have redundant messages that we'll need to keep in sync (particularly for titles).
return new Uint8Array(keyBuffer); | ||
}; | ||
|
||
export const setupNativeFairplayDRM = ( | ||
props: Partial<Pick<MuxMediaPropsInternal, 'playbackId' | 'drmToken' | 'customDomain' | 'drmTypeCb'>>, | ||
props: Partial<Pick<MuxMediaPropsInternal, 'playbackId' | 'tokens' | 'playbackToken' | 'customDomain' | 'drmTypeCb'>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: There's a lot of very similar code for error tracking and translation, both here and with the corresponding hls.js function (below). I couldn't think of any straightforward way to abstract this without even larger refactors, but if anything jumps out, even if minor improvements, I'm definitely amenable.
if (mediaEl.src && (code !== MediaError.MEDIA_ERR_DECODE || code !== undefined)) { | ||
// Attempt to get the response code from the video src url. | ||
try { | ||
const { status } = await fetch(mediaEl.src as RequestInfo); | ||
const { status } = await fetch(mediaEl.src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: I think this should no longer be necessary, since we now fetch playlists (to e.g. infer stream type) in parallel, even for native playback. Those independent requests should also now translate to errors on non-200 response cases.
17d5451
to
ec4560b
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
… DRM and hopefully playback. Expose tokens as props. Some cleanup.
…ck core expectations.
…-core): Clean up tokens and playback ID API and usage across elements and core.
…to playback-core i18n.
…oving core error translation to playback-core.
…grating core architecture to playback-core.
…rors, including added resp info.
…xport for use in e.g. mux-player.
…gories. Update tests.
…ivalent error cases consistent with hls.js error monitoring and translation.
…as hls.js translations.
…r playback-core lang crud.
ec4560b
to
c8c3e1f
Compare
A bunch of changes, motivated by but not exhausted by getting granular mux data drm errors, including:
tokens
to other elements components. This wasn't obviously valuable before DRM, but now it allows for more consistent data structures across different elements "layers""PLAYBACK_ID?query=param1&query2=param2"