6601 load plugin manifest #8485

tsmaeder · 2020-09-09T15:12:14Z

What it does

Fixes "Frontend plugins are broken" #6601 by loading the package.json upon initializing the plugin.

Note: I haven't been able to make the electron case work, since I don't know how to compute the proper URI to load package.json. However, since I'm going to be on vacation, I wanted to propose this PR anyway...maybe someone can fix it.

How to test

Build a front end plugin (for example https://github.com/eclipse/che-theia-samples/tree/master/samples/hello-world-frontend-plugin) and add the resulting plugin to the theia plugins folder.
Start Theia and ensure that the "Hello World" command has been added via F1.

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

paul-marechal

I get an error when running the vscode ext you suggested:

root ERROR Could not transfer 'package.nls.json' file from 'che__eclipse_che_hello_world_frontend_plugin' Error: ENOENT: no such file or directory, stat 'c:\Users\me\AppData\Local\Temp\theia-unpacked\eclipse_che_hello_world_frontend_plugin.theia\package.nls.json'

The ext is indeed missing a package.nls.json file. Is it a mandatory file?

paul-marechal · 2020-09-10T14:25:34Z

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts

+        readPluginJson(pluginModel, 'package.json'),
+        loadTranslations(pluginModel)
+    ]);
+    // translate vscode builtins, as they are published with a prefix. See https://github.com/theia-ide/vscode-builtin-extensions/blob/master/src/republish.js#L50


The link is dead.

Copy paste from the other "plugin-manifest-loader.ts". No Idea where that is coming from.

@akosyakov any idea what the correct link might be here?

cc @marcdumais-work

Since we can't seem to find the proper uri I've removed the link.

I think link is for https://github.com/theia-ide/vscode-builtin-extensions/blob/2997b3e9b2522e48ece117159f88af02b5f8dc7a/src/publish.js#L61

paul-marechal · 2020-09-10T14:28:57Z

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts

+
+function readContents(uri: string): Promise<string> {
+    return new Promise((resolve, reject) => {
+        const request = new XMLHttpRequest();


Any reason why using XHR instead of the newer fetch API?

If all you know is a hammer ;-) No, there's no reason.

I'd prefer using fetch, but I'll leave it up to you since ultimately it works.

benoitf · 2020-09-10T15:17:30Z

I think it's not handling the case where a file could be optional as package.nls.json may be missing

tsmaeder · 2020-09-15T07:35:38Z

I get an error when running the vscode ext you suggested:

root ERROR Could not transfer 'package.nls.json' file from 'che__eclipse_che_hello_world_frontend_plugin' Error: ENOENT: no such file or directory, stat 'c:\Users\me\AppData\Local\Temp\theia-unpacked\eclipse_che_hello_world_frontend_plugin.theia\package.nls.json'

The ext is indeed missing a package.nls.json file. Is it a mandatory file?

No, the error should be handled up the call chain, though.

tsmaeder · 2020-09-21T13:08:11Z

@marechal-p are you using electron? The description states that I have no idea how to compute the URI in this case:

Note: I haven't been able to make the electron case work, since I don't know how to compute the proper URI to load package.json.

If you have any suggestions, I'm all ears. Otherwise, since front-end plugins don't work at all, I think we should still merge the PR, it makes front-end plugins at least work in the browser case.

paul-marechal

@tsmaeder I was convinced I had tried on the browser version, but you might be right. Tested again using browser and it works well. Electron does not.

LGTM.

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts

Signed-off-by: Thomas Mäder <tmader@redhat.com>

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts

vince-fugnitto · 2020-09-25T13:29:49Z

@tsmaeder you can restart the build if you like, sometimes we clear the Travis cache and it helps.

tsmaeder · 2020-09-25T14:06:07Z

I tried restarting the build, but it had no effect. Where do I click?

vince-fugnitto · 2020-09-25T14:06:55Z

I tried restarting the build, but it had no effect. Where do I click?

I'll try for you :)

tsmaeder · 2020-09-25T14:11:24Z

I don't get the "rerun" buttons in the rightmost columns. Who do I need to bribe?

vince-fugnitto · 2020-09-25T14:17:11Z

I tried restarting the build, but it had no effect. Where do I click?

I'm not sure :) @marcdumais-work maybe you know about how to give rights to maintainers for Travis?

tsmaeder · 2020-09-25T14:53:20Z

Now some other test fails and one of the platforms being tested actually passes. This is frustrating!

tsmaeder · 2020-09-25T14:55:15Z

Also, locally on Windows, I get another test failure (pre-hook for "move file" fails). Are the tests expected to be flaky?

marcdumais-work · 2020-09-25T16:06:58Z

I don't get the "rerun" buttons in the rightmost columns. Who do I need to bribe?

You should normally be permitted to do this, since you have write-access to the repo. Can you access to the following? https://travis-ci.com/github/eclipse-theia/theia/settings

vince-fugnitto · 2020-09-25T16:22:39Z

Now some other test fails and one of the platforms being tested actually passes. This is frustrating!

I rebuilt, it previously failed since open-vsx was down and we could not download builtins for the tests.

marcdumais-work · 2020-09-25T16:23:24Z

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts

+        loadTranslations(pluginModel)
+    ]);
+    // translate vscode builtins, as they are published with a prefix.
+    const built_prefix = '@theia/vscode-builtin-';


This is to support using vscode built-ins from npm I think? Do you still use these in Che?
We have a couple of fixes, to work-around problems with specific built-ins during vsix packaging - I do not think we do the same with the npm version.

There is also some extra massaging done to each built-in's package.json, e.g. to make NLS work

marcdumais-work · 2020-09-25T16:28:47Z

Tested again using browser and it works well. Electron does not.

Is this the case that because of the nature of this extension (it's Che-related after all), it's expected not to work on the Electron version of a Theia app?

paul-marechal · 2020-09-25T17:13:20Z

@marcdumais-work more like that in a perfect world frontend extensions should work in any Theia product, but it was broken for both browser and electron contexts. This PR only fixes the browser part (I tested on the example apps), and I don't know how to fix Electron either so hence why I am happy with the current changes.

tsmaeder · 2020-09-28T07:02:40Z

I don't get the "rerun" buttons in the rightmost columns. Who do I need to bribe?

You should normally be permitted to do this, since you have write-access to the repo. Can you access to the following? https://travis-ci.com/github/eclipse-theia/theia/settings

No, I get "Your permissions are insufficient".

marcdumais-work · 2020-09-28T14:42:17Z

"Your permissions are insufficient"

I am not sure what the problem might be - the only hits on Google search seem to be CI for repos that their crawler was enable to access, giving this same error.

One thing worth a try: On the following page, click on "sync account", on the upper left quadrant of the page:
https://travis-ci.com/account/repositories

Do you see the "eclipse-theia" organization listed, under the button?

tsmaeder requested review from akosyakov, paul-marechal and benoitf September 9, 2020 15:12

paul-marechal reviewed Sep 10, 2020

View reviewed changes

tsmaeder requested a review from paul-marechal September 21, 2020 13:03

paul-marechal approved these changes Sep 21, 2020

View reviewed changes

akosyakov added plug-in system issues related to the plug-in system electron issues related to the electron target labels Sep 23, 2020

paul-marechal reviewed Sep 24, 2020

View reviewed changes

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts Outdated Show resolved Hide resolved

Load plugin manifest on activation

622d5cf

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder force-pushed the 6601_load_plugin_manifest branch from 7134444 to 622d5cf Compare September 25, 2020 08:09

paul-marechal approved these changes Sep 25, 2020

View reviewed changes

vince-fugnitto reviewed Sep 25, 2020

View reviewed changes

packages/plugin-ext/src/hosted/browser/worker/plugin-manifest-loader.ts Show resolved Hide resolved

marcdumais-work reviewed Sep 25, 2020

View reviewed changes

tsmaeder merged commit af68b7c into eclipse-theia:master Sep 28, 2020

vince-fugnitto mentioned this pull request Oct 1, 2020

bug: regression caused by 'load plugin manifest' commit #8577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6601 load plugin manifest #8485

6601 load plugin manifest #8485

tsmaeder commented Sep 9, 2020

paul-marechal left a comment

paul-marechal Sep 10, 2020

tsmaeder Sep 15, 2020

tsmaeder Sep 21, 2020

akosyakov Sep 23, 2020

tsmaeder Sep 24, 2020

benoitf Sep 24, 2020

paul-marechal Sep 10, 2020

tsmaeder Sep 15, 2020

paul-marechal Sep 15, 2020

benoitf commented Sep 10, 2020

tsmaeder commented Sep 15, 2020

tsmaeder commented Sep 21, 2020

paul-marechal left a comment

vince-fugnitto commented Sep 25, 2020

tsmaeder commented Sep 25, 2020

vince-fugnitto commented Sep 25, 2020 •

edited

Loading

tsmaeder commented Sep 25, 2020

vince-fugnitto commented Sep 25, 2020

tsmaeder commented Sep 25, 2020

tsmaeder commented Sep 25, 2020

marcdumais-work commented Sep 25, 2020

vince-fugnitto commented Sep 25, 2020

marcdumais-work Sep 25, 2020

marcdumais-work commented Sep 25, 2020

paul-marechal commented Sep 25, 2020 •

edited

Loading

tsmaeder commented Sep 28, 2020

marcdumais-work commented Sep 28, 2020

6601 load plugin manifest #8485

6601 load plugin manifest #8485

Conversation

tsmaeder commented Sep 9, 2020

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Sep 10, 2020

tsmaeder commented Sep 15, 2020

tsmaeder commented Sep 21, 2020

paul-marechal left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Sep 25, 2020

tsmaeder commented Sep 25, 2020

vince-fugnitto commented Sep 25, 2020 • edited Loading

tsmaeder commented Sep 25, 2020

vince-fugnitto commented Sep 25, 2020

tsmaeder commented Sep 25, 2020

tsmaeder commented Sep 25, 2020

marcdumais-work commented Sep 25, 2020

vince-fugnitto commented Sep 25, 2020

Choose a reason for hiding this comment

marcdumais-work commented Sep 25, 2020

paul-marechal commented Sep 25, 2020 • edited Loading

tsmaeder commented Sep 28, 2020

marcdumais-work commented Sep 28, 2020

vince-fugnitto commented Sep 25, 2020 •

edited

Loading

paul-marechal commented Sep 25, 2020 •

edited

Loading