Incorrect "Unsupported activation event" error in stdout #12953

[jfaltermeier]

What it does

In this PR we are adding a mechanism to add additional known activation events to avoid "Unsupported activation event" messages in the log.

In my initial implementation the information is taken from an environment variable.
For the browser application use case the environment may be set when building the container image for the app.
For the electron application use case a custom main script may be used, that sets the desired environment variable value and then hands over to the generated electron-main.js.
Also the user/application may influence the environment variable further if required.

Any additional plugin installed by the user may bring in new warnings, so I was not sure if a more programmatic approach, where the activation events have to be specified at compile/build time, is working for all use cases.
A user preference might work, but it seems a bit weird to me to have this in the settings.
So this is my reasoning for the initial environment variable approach.

If you would prefer a different approach to specify the events, let me know and I will adjust the PR.

Fixes #12953

How to test

Run Electron Example with downloaded plugins

yarn && yarn download:plugins && yarn electron build && clear && yarn electron start

With the default plugins you should get the following warnings:

Unsupported activation events: onProfile, onProfile:github, please open an issue: https://github.com/eclipse-theia/theia/issues/new
Unsupported activation events: onOpenExternalUri:http, onOpenExternalUri:https, please open an issue: https://github.com/eclipse-theia/theia/issues/new
Unsupported activation events: onWalkthrough:nodejsWelcome, please open an issue: https://github.com/eclipse-theia/theia/issues/new

Now run the same same application with the ADDITIONAL_ACTIVATION_EVENTS env variable.

export ADDITIONAL_ACTIVATION_EVENTS=onProfile,onOpenExternalUri,onWalkthrough && clear && yarn electron start

This will remove the warnings/errors.

Follow-ups

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[martin-fleck-at]

Thank you very much Johannes! Change looks good to me in general, I'm just wondering whether we should be so focused on the environment variable approach (see inline comment),

[martin-fleck-at]

I wonder whether the additional activation events shouldn't be part of the PluginManagerInitializeParams used in the $init call to initialize the plugin manager. It already contains the some other setup data and is also called before the $start method with the PluginManagerStartParams where we get the first set of activation events. I think that would also give us a more fine-grained option to handle how those events are obtained and maybe even provide different events for different plugin managers. For now I'm fine if they are still coming from environment variables, just gathered in the hosted-plugin.ts during initialization of the manager and added as a dedicated, optional parameter.

What do you think?

[jfaltermeier]

Sounds good, please have another look at the changes.
I've moved the known events + reading the env var to hosted-plugin.ts and extended the PluginManagerInitializeParams to include the supported events.

[martin-fleck-at]

Great, changes look good to me, thank you Johannes!

[jcortell68]

Thanks, guys. @martin-fleck-at I wonder if the new-and-improved custom plugin API markdown file shouldn't mention this environment variable. Otherwise, app developers will have to debug the error message to find out about the env variable.

[martin-fleck-at]

@jcortell68 That is a very good point! I'll open another PR to add some information on that.

It is here: #13190