[plugin] module resolution needs to be fully qualified

[thegecko]

Bug Description:

TypeScript modules can be exported from index files to make it easier for importing.

For example, this file exports file-uri so that it can be imported like this:

import { FileUri } from '@theia/core/lib/node';

However, plugins in the plugin system don't seem to resolve modules exported in this manner and have to be fully qualified, e,g,:

import { FileUri } from '@theia/core/lib/node/file-uri';

This would be fine if the build fails, however in this case the plugin correctly builds, but fails to load without any error. This situation is prone to lead to some difficult issues to debug.

I would expect either:

• User doesn't need to fully-qualify imports

or

• Imports not fully-qualified would error during compilation of the plugin

Steps to Reproduce:

A repo with a theia plugin outlining the issue is here:

https://github.com/thegecko/theia-plugin

Build this and include it in the plugins when launching Theia. Use these lines to switch between the plugin working and not.

The plugin code to reproduce is short:

import * as theia from '@theia/plugin';
import { FileUri } from '@theia/core/lib/node';     // doesn't work
// import { FileUri } from '@theia/core/lib/node/file-uri';     // works

export const start = async () => {
    const path = FileUri.fsPath(__dirname);
    theia.window.showInformationMessage(path);
};

Additional Information

• Operating System: macOS
• Theia Version: 1.2.0

cc @benoitf Have you seen this issue when writing che plugins?

[akosyakov]

Ideally the plugin host process should not import anything from @theia/* packages to reduce its size. We import though a bit of things to avoid duplications like events, but the rest is not bundled. @theia/core/lib/node/file-uri is working just by luck. You should not use it.

[thegecko]

Ideally the plugin host process should not import anything from @theia/* packages to reduce its size

Of course, I was using this as an example :)

@theia/core/lib/node/file-uri is working just by luck

Oh, so the issue I've outlined is only apparent when importing @theia/core?

I'd like to understand why so that this issue can be avoided for other packages, perhaps it should be documented?

[akosyakov]

Of course, I was using this as an example :)

Oh, you mean that if I install any npm package as a dependency which provides index.js file then it cannot be imported? That's definitely will be a bug.

Oh, so the issue I've outlined is only apparent when importing @theia/core?

I meant that it is not possible to access anything from @theia/* packages by default, since plugins are running in own isolated process.

[thegecko]

Oh, you mean that if I install any npm package as a dependency which provides index.js file then it cannot be imported.

I haven't tried, but can't se it will be any different. My example above only differs in the way things are exported.

plugins are running in own isolated process.

Yeah, I would expect the same, independent classes and/or global functions and types should still work, though.

[thegecko]

I have got to the bottom of this issue.
By importing from the index.js in theia file it also includes cli-manager which is an injectable class and therefore pulls in inversify dependencies.

Inversify requires reflect-metadata to be available, but doesn't fail to build when it's missing. It instead fails at runtime.

In this case, I would expect the plugin which failed to start to exit the entire app just as if this error was raised in the main process.

Therefore, what do people think of crashing the entire application if key plugins fail to start?

This could be implemented for development only using a flag or switch or alternatively, key plugins could be marked as required to start for a clean startup.

[akosyakov]

By importing from the index.js in theia file it also includes cli-manager which is an injectable class and therefore pulls in inversify dependencies.

🙈 It would be good to have a linting rule which restricts dependencies between js modules. VS Code codebase has custom: https://github.com/microsoft/vscode/blob/master/.eslintrc.json#L90 We could adopt it. Additionally we discussed with Sven long time ago to get rid of all index files since they tend to pull everything.

[akosyakov]

We catch the error without propagating to the activation:

theia/packages/plugin-ext/src/hosted/node/plugin-host-rpc.ts

Lines 198 to 200 in 7877a55

	} catch (e) {
	console.error(e);
	}

so failure is never delivered to a user

[thegecko]

We catch the error without propagating to the activation:

I don't see that catch block hit in this case.

This is where the error is swallowed:

theia/packages/plugin-ext/src/hosted/node/plugin-host-rpc.ts

Line 145 in 7877a55

console.error(e);

[akosyakov]

@thegecko this catch can be removed and PluginManagerExtImpl.loadPlugin should catch an exception and handle it. Probably we should reject the activation promise as well in this case as here:

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

Lines 366 to 373 in 3eb0678

	} catch (err) {
	if (this.pluginActivationPromises.has(plugin.model.id)) {
	this.pluginActivationPromises.get(plugin.model.id)!.reject(err);
	}
	this.messageRegistryProxy.$showMessage(MainMessageType.Error, `Activating extension ${id} failed: ${err.message}.`, {}, []);
	console.error(`Error on activation of ${plugin.model.name}`, err);
	return false;
	}

Maybe the catch block should be moved to PluginManagerExtImpl.loadPlugin as well.