Skip to content
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

[Proposal] Forbid usage of the MediaKeys type and other EME TS types #1477

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Jul 5, 2024

Based on #1397.

In the idea of including fake encrypted contents in our integration tests by mocking MSE+EME+HTML 5 media API (not as complex as it sounds) to increase by a lot the types of issues our CI is capable to catch, I noticed an opportunity to even improve on the current code.

Like in #1397, the idea is there to provide typing subsets for the various EME API and to rely on them in the RxPlayer code (and enforcing this through our linter).

This allows to:

  • much simplify EME API mocking by not having to implement the full extent of the EME API (though what is missing in the custom types are very minor here: the EventTarget's third options optional parameter which we never use, dispatchEvent, and the onevent methods that we never rely on).

  • Allow the definition of environment-specific APIs - such as webkit-vendored API

  • Be more aware of which EME API we currently use

The gain is here much more evident than in #1397 as we already had some kind of subtyping with the ICustom... types (e.g. ICustomMediaKeys). By renaming those I... (e.g. IMediaKeys) and ensuring they are actually compatible with the base type (e.g. MediaKeys), we end up in my opinion with simpler code.

@peaBerberian peaBerberian added DRM Relative to DRM (EncryptedMediaExtensions) proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it labels Jul 5, 2024
@peaBerberian peaBerberian added this to the 4.2.0 milestone Jul 9, 2024
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from c4aae81 to a7cc348 Compare July 24, 2024 16:49
@peaBerberian peaBerberian added the Priority: 3 (Low) This issue or PR has a low priority. label Jul 26, 2024
@peaBerberian peaBerberian changed the base branch from misc/imediaelement to dev July 31, 2024 09:07
src/compat/browser_compatibility_types.ts Outdated Show resolved Hide resolved
Comment on lines 84 to +85
const onSessionRelatedEvent = (evt: Event) => {
this.trigger(evt.type, evt);
this.trigger(evt.type as keyof MediaKeySessionEventMap, evt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we can type onSessionRelatedEvent with MediaKeyMessageEvent | Event

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

line 84? What would that change?

Copy link
Collaborator

@Florent-Bouisset Florent-Bouisset Aug 1, 2024

Choose a reason for hiding this comment

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

I mean events like "keymessage", "message", "keyadded", "ready", "keyerror", "error" seems to be typed as a generic Event but it can be more precize with a type MediaKeyMessageEvent.

I don't really understand why typescript correctly show an error for

 this.trigger("message", evt);

but does not trigger an error for

this.trigger(evt.type as keyof MediaKeySessionEventMap, evt);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how to fix this, if you have an idea, you can commit it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about how is it supposed to work on the "old webkit", from what I read, we can have an event
triggered from error with a type that is different, it seems weird ?

mediaElement.addEventListener("error", (event) => event.type /* keystatuseschange*/)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

? How is it possible? Where did you see that possibility?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought that event "type" was more precised in typescript but it's always a string
In fact what we can do is:

    const onSessionRelatedEvent = (evt: MediaKeyMessageEvent | Event) => {
      this.trigger(evt.type, evt);
    };

but I'm not sure if it's better than casting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I'm kind of lost on that one.

In any case, the compat EME files are the "ugliest" TypeScript files in the RxPlayer to me but it makes sense that it is (due to the beauty of EME implementations out there).

@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from e210921 to 6429671 Compare August 1, 2024 13:53
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from 231f5b1 to d52f7b9 Compare August 13, 2024 16:50
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 3 times, most recently from 74fed2d to 29e63ac Compare September 4, 2024 17:53
@peaBerberian peaBerberian modified the milestones: 4.2.0, 4.3.0 Oct 4, 2024
Based on #1397.

In the idea of including fake encrypted contents in our integration
tests by mocking MSE+EME+HTML 5 media API (not as complex as it sounds)
to increase by a lot the types of issues our CI is capable to catch, I
noticed an opportunity to even improve on the current code.

Like in #1397, the idea is there to provide typings subset of the
various EME API and to rely on them instead in the RxPlayer code (and
enforcing this through our linter).

This allows to:

  - much simplify EME API mocking by not having to implement the full
    extent of the EME API - though almost all of it is implemented
    instead (exceptions are the `EventTarget`'s third `options` optional
    parameter which we never use, `dispatchEvent`, and the `onevent`
    methods that we never rely on).

  - Allow the definition of environment-specific APIs

  - Be more aware of which EME API we currently use

The gain is here much more evident than in #1397 as we already had some
kind of sub-typings with the `ICustom...` types (e.g.
`ICustomMediaKeys`). By renaming those `I...` (e.g. `IMediaKeys`) and
ensuring they are actually compatible with the base type (e.g.
`MediaKeys`), we end up in my opinion with simpler code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DRM Relative to DRM (EncryptedMediaExtensions) Priority: 3 (Low) This issue or PR has a low priority. proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants