-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix(FEC-10776): set the plugins event registration after kaltura player internal events #383
Conversation
Issue: plugins register before ui/provider and internal handler in kaltura player which could cause incorrect behoivor in the player after plugins handler. Solution: configure the plugins after internal logic. For example - when plugin register on constractor to error and make reset it'll handle the event before ui/provider and kaltura player handle it and cause the error overlay to appear, cause reset didn't clear this state after it changed.
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.
LGTM.
@yairans do you see any risks with plugins and UI?
@@ -75,6 +74,7 @@ class KalturaPlayer extends FakeEventTarget { | |||
Object.values(CoreEventType).forEach(coreEvent => this._eventManager.listen(this._localPlayer, coreEvent, e => this.dispatchEvent(e))); | |||
this._addBindings(); | |||
this._playlistManager.configure(options.playlist); | |||
this.configure({plugins}); |
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.
The UIWrapper
uses the player.uiComponents
which filled by the plugins (by getUIComponents
) while their initialization. So we can't move it after the UIWrapper
initialization.
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.
So the correct flow here is to actually fix the UI manager issue.
@Yuvalke the correct expectation is that if someone calls reset on player it will actually reset the UI as well, even if it is in transit.
Issue is that as we call reset on error event and event didn't yet reached UI we can't really stop it - chicken and egg kind of situation.
I suggest you and @yairans discuss this so @yairans can have more context and let's try to figure it out.
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.
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.
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.
@yairans updated as we discuss, external UI components will update via config and plugins will update via API.
@@ -16,7 +16,6 @@ class UIWrapper { | |||
|
|||
constructor(player: KalturaPlayer, options: KPOptionsObject) { | |||
const config: KPUIOptionsObject = options.ui; | |||
config.uiComponents = [...(player.uiComponents || []), ...(config.uiComponents || [])]; |
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.
it is possible for app to add components as well so can't remove this
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.
@OrenMe why not? it just merges the plugins uiComponents with config uiComponents, it shouldn't change the config and then uiComponents will pass as is
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.
need to make sure that if app passes the uiComponents config it is passed.
We only discussed the plugins which is internal, but an app can do:
KalturaPlayer.setup({
plugins: {
uiComponents: []
}
})
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, sure. Why not? Nobody remove it, it pass on config directly to ui manager as it was before.
Those lines merging the uiComponents from plugins and config.uiComponents which doesn't need it anymore, its done by addComponent API instead and config pass as is.
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.
KalturaPlayer.setup({
ui: {
uiComponents: []
}
})
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.
So player.uiComponents
is useless. maybe we can remove it (don't think anyone uses 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.
Didn't remove it since it's API, once someone change it, it's breaking changes
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 think we only used it when we passed the uiComponents from playkit when plugins were in it.
#264
It is safe to remove as we alway supported adding it only on initial config until we recently added the dynamic injection API which has it's own code paths.
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 if customers use it as getter, anyway if it's ok by you I'll remove 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.
@esakal can you please review the removal of uiComponents
getter.
It seems this is only used for setting it but maybe contrib
is using it?
Description of the Changes
Issue: plugins register before UI/provider and internal handler in Kaltura player which could cause incorrect behavior in the player after plugins handler.
Solution: configure the plugins after internal logic - seems we would like to handled last.
UI components that come from plugins adding by addComponent API so external UI components of Kaltura player will update via config.
For example - when the plugin registers on the contractor to error and makes a reset it'll handle the event before UI/provider and Kaltura player handle it and cause the error overlay to appear on UI, plugin call to reset didn't clear this state after it changed, it cleared it before UI changed it and does nothing.
CheckLists