-
Notifications
You must be signed in to change notification settings - Fork 133
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
base: dev
Are you sure you want to change the base?
Conversation
4e16285
to
4d229b6
Compare
4e190ac
to
6e4f495
Compare
4d229b6
to
2d3dedb
Compare
2e8a86a
to
ee99661
Compare
c4aae81
to
a7cc348
Compare
a7cc348
to
2fbc31b
Compare
const onSessionRelatedEvent = (evt: Event) => { | ||
this.trigger(evt.type, evt); | ||
this.trigger(evt.type as keyof MediaKeySessionEventMap, evt); |
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'm wondering if we can type onSessionRelatedEvent with MediaKeyMessageEvent | Event
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.
line 84? What would that change?
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 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);
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 don't know how to fix this, if you have an idea, you can commit it
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'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*/)
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.
? How is it possible? Where did you see that possibility?
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 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
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.
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).
e210921
to
6429671
Compare
6429671
to
ce097d5
Compare
231f5b1
to
d52f7b9
Compare
74fed2d
to
29e63ac
Compare
29e63ac
to
13d94f1
Compare
13d94f1
to
d5e9d37
Compare
d5e9d37
to
93465f5
Compare
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.
93465f5
to
6cb2c4b
Compare
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 thirdoptions
optional parameter which we never use,dispatchEvent
, and theonevent
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 thoseI...
(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.