fix(FEC-10776): set the plugins event registration after kaltura player internal events #383

Yuvalke · 2020-11-26T15:43:01Z

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

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
test are passing in local environment
Travis tests are passing (or test results are not worse than on master branch :))
Docs have been updated

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.

OrenMe

LGTM.
@yairans do you see any risks with plugins and UI?

yairans · 2020-12-07T16:55:34Z

src/kaltura-player.js

@@ -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});


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.

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.

@yairans @OrenMe we can pass on configure with setConfig and pass the uiComponents there instead, I'll need to verify it with plugins like this

@Yuvalke plugins expose UI components, it is not just app so need them to be available before.
But now with dynamic injection this is not true any more and I assume we can find a solution where UI is configured and then plugins and then we register plugins UI components on UI.
@yairans FYI

@yairans updated as we discuss, external UI components will update via config and plugins will update via API.

OrenMe · 2020-12-08T08:37:54Z

src/common/ui-wrapper.js

@@ -16,7 +16,6 @@ class UIWrapper {

  constructor(player: KalturaPlayer, options: KPOptionsObject) {
    const config: KPUIOptionsObject = options.ui;
-    config.uiComponents = [...(player.uiComponents || []), ...(config.uiComponents || [])];


it is possible for app to add components as well so can't remove this

@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

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: [] } })

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.

KalturaPlayer.setup({ ui: { uiComponents: [] } })

@OrenMe

So player.uiComponents is useless. maybe we can remove it (don't think anyone uses it)

Didn't remove it since it's API, once someone change it, it's breaking changes

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.

I mean if customers use it as getter, anyway if it's ok by you I'll remove it

@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?

…ltura-player-js into fix-order-of-register

Yuvalke requested a review from a team November 26, 2020 15:43

Yuvalke self-assigned this Nov 26, 2020

OrenMe previously approved these changes Dec 7, 2020

View reviewed changes

yairans suggested changes Dec 7, 2020

View reviewed changes

add component by uiWrapper API

f49df61

Yuvalke dismissed OrenMe’s stale review via f49df61 December 7, 2020 21:53

Yuvalke added 2 commits December 8, 2020 00:02

Merge branch 'master' into fix-order-of-register

019b1d9

Update ui-wrapper.js

f0e91ab

OrenMe suggested changes Dec 8, 2020

View reviewed changes

Yuvalke and others added 2 commits December 8, 2020 10:38

Update kaltura-player.js

2b8b593

Merge branch 'master' into fix-order-of-register

78e9050

Yuvalke changed the title ~~fix: set the plugins event registration after kaltura player internal events~~ fix(FEC-10776): set the plugins event registration after kaltura player internal events Dec 8, 2020

Yuvalke and others added 3 commits December 8, 2020 13:22

Merge branch 'master' into fix-order-of-register

8b9f5a1

remove uiComponent getter

c00063e

Merge branch 'fix-order-of-register' of https://github.com/kaltura/ka…

0ad51c5

…ltura-player-js into fix-order-of-register

OrenMe requested a review from esakal December 8, 2020 13:20

Merge branch 'master' into fix-order-of-register

448f4b7

yairans approved these changes Dec 10, 2020

View reviewed changes

OrenMe approved these changes Dec 10, 2020

View reviewed changes

Yuvalke merged commit 4233d9f into master Dec 10, 2020

Yuvalke deleted the fix-order-of-register branch December 10, 2020 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(FEC-10776): set the plugins event registration after kaltura player internal events #383

fix(FEC-10776): set the plugins event registration after kaltura player internal events #383

Yuvalke commented Nov 26, 2020 •

edited

Loading

OrenMe left a comment

yairans Dec 7, 2020 •

edited

Loading

OrenMe Dec 7, 2020

Yuvalke Dec 7, 2020 •

edited

Loading

OrenMe Dec 7, 2020

Yuvalke Dec 8, 2020

OrenMe Dec 8, 2020

Yuvalke Dec 8, 2020 •

edited

Loading

OrenMe Dec 8, 2020

Yuvalke Dec 8, 2020 •

edited

Loading

yairans Dec 8, 2020 •

edited

Loading

yairans Dec 8, 2020

Yuvalke Dec 8, 2020 •

edited

Loading

OrenMe Dec 8, 2020

Yuvalke Dec 8, 2020

OrenMe Dec 8, 2020

fix(FEC-10776): set the plugins event registration after kaltura player internal events #383

fix(FEC-10776): set the plugins event registration after kaltura player internal events #383

Conversation

Yuvalke commented Nov 26, 2020 • edited Loading

Description of the Changes

CheckLists

OrenMe left a comment

Choose a reason for hiding this comment

yairans Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

yairans Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke commented Nov 26, 2020 •

edited

Loading

yairans Dec 7, 2020 •

edited

Loading

Yuvalke Dec 7, 2020 •

edited

Loading

Yuvalke Dec 8, 2020 •

edited

Loading

Yuvalke Dec 8, 2020 •

edited

Loading

yairans Dec 8, 2020 •

edited

Loading

Yuvalke Dec 8, 2020 •

edited

Loading