-
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
Add onStartupFinished
plugin activation event
#8525
Conversation
@@ -217,6 +217,7 @@ export class HostedPluginSupport { | |||
}; | |||
} | |||
}); | |||
this.activateByEvent('onStartupFinished'); |
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 was wondering why the event should be sent at this location and not another. What parts of the system need to be ready in order for startup to be finished? Not saying it's the wrong place, I just want to understand your thinking.
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've verified it according to "How to test" steps. Works well, but I have some concerns about the activation order.
Why not emit it after a star(*) activation
await this.$activateByEvent('*'); |
as it's in VSCode
https://github.com/microsoft/vscode/blob/0e297b112830bd5be9cf51a848d9314c3887ec42/src/vs/workbench/api/common/extHostExtensionService.ts#L475
Isn't it too early emitting onStartupFinished
during hosted-plugin initialization?
Won't it happen eager than a star(*) activation?
65d4841
to
d3f1912
Compare
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.
Or another variant. As HostedPluginSupport
is FrontendApplicationContribution
, wouldn't it better to emit the event on some of the FrontendApplicationContribution
's callbacks, e.g. onStart
or onDidInitializeLayout
?
@tsmaeder @azatsarynnyy I think I've found a better place for this event :). Now it is called on application's |
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 believe emitting on appState.reachedState('ready')
looks late enough as for onStartupFinished
.
Did you check VS Code source code to learn the real semantic of this event and align it? |
@akosyakov theia/packages/plugin-ext/src/hosted/browser/hosted-plugin.ts Lines 415 to 422 in 8bf15dc
|
yes, i think it should mean the same |
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
d3f1912
to
4dabbce
Compare
@akosyakov Done |
@vinokurig I can see the following error message with latest master: root ERROR [hosted-plugin: 93564] Unsupported activation events: onStartupFinished, please open an issue: https://github.com/eclipse-theia/theia/issues/new Is there something specific to do to avoid that? |
@jeluard Thank you for reporting the issue, the PR above fixes this problem. |
What it does
Add
onStartupFinished
plugin activation eventfixes #8488
How to test
package.json
from*
toonStartupFinished
Review checklist
Reminder for reviewers