[vscode] basic support of activation events #5622

akosyakov · 2019-07-02T13:39:11Z

What it does

[vscode] support activation events #4199 - provides basic mechanism for activation events:
- compatibility: if an activation event is not supported yet then an extension will be activated eagerly with *
- support of onLanguage activation event
- support of onCommand activation event

How to test

unsupported events: deploy vscode go extension and check that it is deployed eagerly since it has unsupported activation events
onLanguage event: deploy https://github.com/akosyakov/lang-activation-event/blob/master/lang-activation-event-0.0.1.vsix and test that Hello World notification appears only when typescript file is opened
onCommand event: deploy https://github.com/akosyakov/command-activation-event/blob/master/command-activation-event-0.0.1.vsix and test that Hello World command is available without a plugin activation and that it shows Hello World notification on the execution

See how to deploy vscode extension here: https://github.com/theia-ide/theia/wiki/Testing-VS-Code-Extensions

Review Checklist

akosyakov · 2019-07-02T13:39:51Z

@evidolob Could you assign someone to help with the review?

akosyakov · 2019-07-02T13:41:35Z

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

-    private frontendExtManagerProxy: PluginManagerExt;
-    private backendExtManagerProxy: PluginManagerExt;
+
+    protected readonly managers: PluginManagerExt[] = [];


@evidolob @benoitf I've refactored this code to support multiple managers properly. Before one backend manager will receive events from the frontend. Are you fine with such breaking changes? I will update the changelog.

I think refactoring and new feature should be separate commits or separate PR
moving from constructor injection to property injection is not directly linked to that PR so it makes a lot of extra changes for nothing.
I think this is kind of PR that I dislike the more when there is one change that is important but it's flooded by tons of cleanup/style/user preferences that should be just for that.
It's like something can't be added without changing all the code which usually is not so true.

BTW 'properly' is not meaningful. You should describe why it was limited and how you've removed that limitation so that people using that class are able to see what was the goal. This is more meaningful for me that a checklist of 100 items which are not describing the why, how and what.

besides that it's a great feature and I'm ok with the refactoring. But breaking change doesn't mean to mix enhancement/bugfixes with code cleanup/rewrite for its own style preference, etc the class in one step. It's so inconvenient to review.

I think refactoring and new feature should be separate commits or separate PR
moving from constructor injection to property injection is not directly linked to that PR so it makes a lot of extra changes for nothing.
I think this is kind of PR that I dislike the more when there is one change that is important but it's flooded by tons of cleanup/style/user preferences that should be just for that.
It's like something can't be added without changing all the code which usually is not so true.

Generally agree that PRs should be to the point. Not sure about refactoring PRs itself. It is fine if there is good set of regression tests, but if there are not it is an extra effort to test again. I would try to make additional explicit commits for refactoring next time or better to look in the test infrastructure next weeks.

BTW 'properly' is not meaningful. You should describe why it was limited and how you've removed that limitation so that people using that class are able to see what was the goal. This is more meaningful for me that a checklist of 100 items which are not describing the why, how and what.

To be fair I've tried to describe it with: Before one backend manager will receive events from the frontend.. Maybe it was not clear: now all remote plugin host processes get notified about updated storage path and activation events. Agree that i had to put it in the description.

packages/plugin-ext/src/hosted/browser/plugin-activation-event-service.ts

packages/plugin-ext/src/hosted/browser/plugin-host-manager.ts

AlexTugarev · 2019-07-02T13:49:41Z

Great preparation for a review @akosyakov! 🥇

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov · 2019-07-03T10:44:15Z

I've pushed support of onCommand activation event. We need to check that there is no regressions for theia plugins which don't use package metadata but register command programatically. @benoitf @evidolob Could you recommend theia plugin for it? @azatsarynnyy maybe you can help to verify it

kittaakos · 2019-07-03T11:08:00Z

@akosyakov, your last commit seems to broke the build: https://github.com/theia-ide/theia/pull/5622/files#diff-ab6e2d4c57658cf60e550cfe9ba87eacR291

akosyakov · 2019-07-03T12:28:48Z

@kittaakos pushed again, should be good now

azatsarynnyy · 2019-07-03T12:42:34Z

@akosyakov I've modified your extension here https://github.com/akosyakov/command-activation-event/blob/master/src/extension.ts#L20 to call another command

vscode.commands.executeCommand('extension.helloWorld2');

instead of showing a message.

Then I've forked your extension and slightly modified it
akosyakov/command-activation-event@master...azatsarynnyy:master

So, calling Hello World command from the 1'st extension via Cmd Palette should call extension.helloWorld2 command from the 2'nd extension which is the activation event for the 2'nd extension.
But calling Hello World command throws

root ERROR [hosted-plugin: 25339] (node:25339) UnhandledPromiseRejectionWarning: Error: The command 'extension.helloWorld2' cannot be executed. There are no active handlers available for the command.
    at CommandRegistry.<anonymous> (http://localhost:3000/bundle.js:107193:31)
    at step (http://localhost:3000/bundle.js:106971:23)
    at Object.next (http://localhost:3000/bundle.js:106952:53)
    at fulfilled (http://localhost:3000/bundle.js:106943:58)

kittaakos · 2019-07-03T12:55:10Z

Fixed

I got this after installing the go extension as suggested in the description and I clicked on:

Uncaught (in promise) Error: The command 'go.promptforinstall' cannot be executed. There are no active handlers available for the command.
    at CommandRegistry.<anonymous> (command.ts:285)
    at step (command.ts:15)
    at Object.next (command.ts:15)
    at fulfilled (command.ts:15)

kittaakos · 2019-07-03T13:07:44Z

[out-of-scope?]: I do not know if this is a bug or the desired behavior, but I put all three above-mentioned plug-ins into the plugins folder and I can see:

root INFO [hosted-plugin: 55804] PLUGIN_HOST(55804): initializing(/Users/akos.kitta/git/theia/packages/plugin-ext-vscode/lib/node/plugin-vscode-init.js)

root INFO [hosted-plugin: 55804] PLUGIN_HOST(55804): initializing(/Users/akos.kitta/git/theia/packages/plugin-ext-vscode/lib/node/plugin-vscode-init.js)

root INFO [hosted-plugin: 55804] PLUGIN_HOST(55804): initializing(/Users/akos.kitta/git/theia/packages/plugin-ext-vscode/lib/node/plugin-vscode-init.js)

I wonder why do we log the same things three times.

kittaakos

I read through the code, added a few remarks here and there. I also tried it with the command, language, and the "wildcard" activation. They've worked. 🎉 Let's improve the documentation for the new command API.

packages/core/src/common/command.ts

kittaakos · 2019-07-03T11:17:21Z

packages/core/src/common/command.ts

+    /**
+     * An event is emmited when a command is about to be executed.
+     *
+     * It can be used to install or active a command handler.


typo: or activate

packages/plugin-ext/src/main/browser/plugin-contribution-handler.ts

packages/plugin-ext/src/plugin/plugin-manager.ts

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

packages/plugin-ext/src/api/plugin-api.ts

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

packages/core/src/common/command.ts

akosyakov · 2019-07-03T14:12:09Z

@kittaakos could you file an issue for #5622 (comment)? I don't know either.

akosyakov · 2019-07-03T14:59:39Z

@azatsarynnyy it cannot work, since command declaration is missing, It is mandatory in VS Code. I will verify it. registerCommand works differently for theia and vscode namespaces. For theia it registers a command, for vscode it registers a handler.

akosyakov · 2019-07-03T15:05:22Z

@azatsarynnyy oh, actually it does work in VS Code 😄

akosyakov · 2019-07-03T15:36:11Z

@azatsarynnyy could you try again? I've adjusted logic how we register commands for vscode namespace, now only if a command is presented in the metadata it will be registered as a handler, otherwise like a command and handler. See https://github.com/theia-ide/theia/pull/5622/files#diff-66cd2825aa616ad9a26b1f62e1b21e5b

Super simple and good test btw. It verified theia plugins and found new and existing bugs :)

akosyakov · 2019-07-03T15:42:45Z

@kittaakos Can you try again #5622 (comment)? I cannot reproduce it in Gitpod, will try locally. But probably it is fixed by #5622 (comment) as well

I was not able to reproduce it locally as well:

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

kittaakos · 2019-07-04T07:24:03Z

Can you try again #5622 (comment)?

No errors this time 👍

kittaakos

I have verified it once more locally. I have checked the followings:

go extension,
activation by language, and
activation by command.

It worked. I did not see any errors in the logs. Very nice 👍

PS: Thank you for adding the empty lines 😊

azatsarynnyy

@akosyakov I've tested it again with two plugins and plugin activation works well

benoitf

tested with one che-theia example

akosyakov requested review from benoitf and evidolob as code owners July 2, 2019 13:39

akosyakov requested a review from AlexTugarev July 2, 2019 13:39

akosyakov commented Jul 2, 2019

View reviewed changes

packages/plugin-ext/src/hosted/browser/plugin-activation-event-service.ts Outdated Show resolved Hide resolved

akosyakov commented Jul 2, 2019

View reviewed changes

packages/plugin-ext/src/hosted/browser/plugin-host-manager.ts Outdated Show resolved Hide resolved

akosyakov force-pushed the ak/basic_activation_event branch from d6a0467 to cf111db Compare July 2, 2019 13:44

akosyakov force-pushed the ak/basic_activation_event branch from cf111db to 9a93def Compare July 2, 2019 13:50

akosyakov requested a review from tolusha July 3, 2019 07:27

akosyakov added 2 commits July 3, 2019 10:39

[vscode] support any activation event

f9100a6

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[vscode] support onLanguage activation event

324dcea

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/basic_activation_event branch from 9a93def to fea1750 Compare July 3, 2019 10:39

akosyakov requested review from azatsarynnyy and kittaakos July 3, 2019 10:44

akosyakov force-pushed the ak/basic_activation_event branch from fea1750 to 0d53466 Compare July 3, 2019 12:28

kittaakos reviewed Jul 3, 2019

View reviewed changes

akosyakov force-pushed the ak/basic_activation_event branch from 0d53466 to 1ca8d99 Compare July 3, 2019 14:50

akosyakov force-pushed the ak/basic_activation_event branch from 1ca8d99 to 45a4578 Compare July 3, 2019 15:33

[vscode] support onCommand activation event

6e63ef7

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/basic_activation_event branch from 45a4578 to 6e63ef7 Compare July 3, 2019 16:08

akosyakov added the vscode issues related to VSCode compatibility label Jul 4, 2019

kittaakos self-requested a review July 4, 2019 07:27

kittaakos approved these changes Jul 4, 2019

View reviewed changes

azatsarynnyy approved these changes Jul 4, 2019

View reviewed changes

benoitf approved these changes Jul 4, 2019

View reviewed changes

kittaakos mentioned this pull request Jul 4, 2019

Adjusted the plug-in related logging. #5637

Merged

akosyakov mentioned this pull request Jul 4, 2019

improve PR reviews #5616

Merged

1 task

akosyakov merged commit b39a852 into master Jul 4, 2019

akosyakov deleted the ak/basic_activation_event branch July 4, 2019 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vscode] basic support of activation events #5622

[vscode] basic support of activation events #5622

akosyakov commented Jul 2, 2019 •

edited

Loading

akosyakov commented Jul 2, 2019

akosyakov Jul 2, 2019

benoitf Jul 4, 2019

akosyakov Jul 4, 2019

AlexTugarev commented Jul 2, 2019

akosyakov commented Jul 3, 2019

kittaakos commented Jul 3, 2019

akosyakov commented Jul 3, 2019

azatsarynnyy commented Jul 3, 2019

kittaakos commented Jul 3, 2019 •

edited

Loading

kittaakos commented Jul 3, 2019

kittaakos left a comment

kittaakos Jul 3, 2019

akosyakov Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019 •

edited

Loading

kittaakos commented Jul 4, 2019

kittaakos left a comment

azatsarynnyy left a comment

benoitf left a comment

[vscode] basic support of activation events #5622

[vscode] basic support of activation events #5622

Conversation

akosyakov commented Jul 2, 2019 • edited Loading

What it does

How to test

Review Checklist

akosyakov commented Jul 2, 2019

akosyakov Jul 2, 2019

Choose a reason for hiding this comment

benoitf Jul 4, 2019

Choose a reason for hiding this comment

akosyakov Jul 4, 2019

Choose a reason for hiding this comment

AlexTugarev commented Jul 2, 2019

akosyakov commented Jul 3, 2019

kittaakos commented Jul 3, 2019

akosyakov commented Jul 3, 2019

azatsarynnyy commented Jul 3, 2019

kittaakos commented Jul 3, 2019 • edited Loading

kittaakos commented Jul 3, 2019

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos Jul 3, 2019

Choose a reason for hiding this comment

akosyakov Jul 3, 2019

Choose a reason for hiding this comment

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019

akosyakov commented Jul 3, 2019 • edited Loading

kittaakos commented Jul 4, 2019

kittaakos left a comment

Choose a reason for hiding this comment

azatsarynnyy left a comment

Choose a reason for hiding this comment

benoitf left a comment

Choose a reason for hiding this comment

akosyakov commented Jul 2, 2019 •

edited

Loading

kittaakos commented Jul 3, 2019 •

edited

Loading

akosyakov commented Jul 3, 2019 •

edited

Loading