EME Controller TypeScript #2076
Conversation
|
Link #2070 |
|
Blocked by #2075. |
|
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.
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.
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 | |||
Added TypeScript types to the code. Handled media detached and removing the encrypted event listener.
|
This is squashed and ready to be reviewed @tjenkinson. |
| * @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.
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.
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}`); |
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. |
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
Windowtype. It defines it as a global interface that can be used.I left the
onManifestParsedevent 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 functiongetSupportedMediaKeySystemConfigurationswill never return null. It either throws or returns a keysystem config. So theif (!never would be fired, so I removed it.Checklist