-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 Issue 8463 - calling extension activate() by other extension #8542
Fix Issue 8463 - calling extension activate() by other extension #8542
Conversation
@akosyakov @amiramw can you please assign yourself as reviewers? |
@@ -412,9 +412,10 @@ export class PluginManagerExtImpl implements PluginManagerExt, PluginManager { | |||
|
|||
if (this.activatedPlugins.get(pluginId)) { | |||
deferred.resolve(); | |||
return deferred.promise; |
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.
deferred seems redundant in this function as it does not defer to something asynchronous
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 didn't added the deferred. do you suggest removing it?
} | ||
this.pluginActivationPromises.set(pluginId, deferred); | ||
return deferred.promise; | ||
return this.$activatePlugin(pluginId); |
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.
this.activatedPlugins
should be updated so if it is called twice you return it from there, right?
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's being updated at startPlugin() method
@@ -412,9 +412,10 @@ export class PluginManagerExtImpl implements PluginManagerExt, PluginManager { | |||
|
|||
if (this.activatedPlugins.get(pluginId)) { | |||
deferred.resolve(); | |||
return deferred.promise; | |||
} | |||
this.pluginActivationPromises.set(pluginId, deferred); |
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.
is this map set only when calling activate plugin programatically?
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.
No, it's being set for all activate plugin methods.
@tomer-epstein @akosyakov @svor @benoitf Looking at the code again I do not understand why we maintain If we are worried about load flow running twice in parallel for the same plugins then there is already a check for that here:
|
|
…n't work. Signed-off-by: Tomer Epstein <tomer.epstein@sap.com>
test scenario added.
|
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.
Code look good to me now and tested successfully both scenarios.
If someone else can also take a look that would be great.
Fix Issue 8463 - calling extension activate() by other extension doesn't work.
Signed-off-by: Tomer Epstein tomer.epstein@sap.com
Issue: #8463
What it does
This change proposal adds a call to activate plugin when ext isn't active and developer called extension.activate() method.
How to test
Review checklist
Reminder for reviewers