-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
EME Controller TypeScript #2076
EME Controller TypeScript #2076
Conversation
Link #2070 |
Blocked by #2075. |
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.
nice 🎉
I think I addressed your comments @tjenkinson (oops) and added the same strictNullChecks: true locally as in #2073 to catch the other usages of |
src/controller/eme-controller.ts
Outdated
|
||
return url; | ||
return null; |
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.
Maybe this could be for a future pr, but it might be better to throw in this case and then we don't have to return null, or handle null later
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.
I did it in this PR. It didn't touch any external APIs and also made the typings make more sense so I could justify it to myself 👍
src/controller/eme-controller.ts
Outdated
@@ -451,14 +478,25 @@ class EMEController extends EventHandler { | |||
} | |||
|
|||
const url = this.getLicenseServerUrl(keysListItem.mediaKeySystemDomain); | |||
if (!url) { | |||
return; // error handling occurs in function and returns null |
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.
See earlier comment
Added TypeScript types to the code. Handled media detached and removing the encrypted event listener.
This is squashed and ready to be reviewed @tjenkinson. |
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.
Looks good to me!
* @param {ArrayBuffer} keyMessage Message data issued by key-system | ||
* @param {function} callback Called when XHR has succeeded | ||
*/ | ||
private _onLicenseRequestReadyStageChange (xhr: XMLHttpRequest, url: string, keyMessage: ArrayBuffer, callback: (data: ArrayBuffer) => void) { | ||
switch (xhr.readyState) { |
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.
This doesn't need to be a switch, but appreciate this isn't a change in this pr
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.
Yeah. I've attempted to keep any changes to code structure in these to simplify null checks, which is why you see me moving code into early returns style vs wrapping code in conditionals. Since if those conditionals are the potentially null values, the type-checker proves to itself that they can't be null.
Same reason I ended up going with throwing and doing the catch in the below example was it made the resulting code much more terse, and type-safe.
const challenge = this._generateLicenseRequestChallenge(keysListItem, keyMessage); | ||
xhr.send(challenge); | ||
} catch (e) { | ||
logger.error(`Failure requesting DRM license: ${e}`); |
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.
👍
Clears the encrypted event listener from the HTMLMediaElement in controller, and releases the stored reference.
@michaelcunningham19 if you could re-run the Travis CI. It looks like it died on this run. However, the Angel One functional test on the last run before the commit 30 minutes ago it was successful (https://travis-ci.org/video-dev/hls.js/builds/483043612). Looks like there were network issues this time. Thanks. I think I've addressed everything in @tjenkinson's review. I'm ready to have this one merged once the functional tests pass on CI. |
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.
@itsjamie Yeah tests are passing now 👍
LGTM 🌮
This PR will...
This adds TypeScript definitions to the eme controller to help migrate the internals of hls.js over to TypeScript.
Are there any points in the code the reviewer needs to double check?
I added the browser environment to .eslintrc so that XMLHttpRequest is defined globally because there was a fight between eslint and typescript. TypeScript doesn't define XMLHttpRequest off the
Window
type. It defines it as a global interface that can be used.I left the
onManifestParsed
event handler untyped because I know @johnBartos is planning on overhauling the manifest parsing types so this can wait to be typed when those are in master.The other double-check part is a change in
_attemptKeySystemAccess
. The functiongetSupportedMediaKeySystemConfigurations
will never return null. It either throws or returns a keysystem config. So theif (!
never would be fired, so I removed it.Checklist