Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add plugins resources provider contribution point #6070

Closed
wants to merge 1 commit into from
Closed

Add plugins resources provider contribution point #6070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/plugin-ext/src/common/plugin-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,25 @@ export interface MetadataProcessor {
process(pluginMetadata: PluginMetadata): void;
}

/**
* Designed to retreive plugins resources from non default or remote location.
*/
export const PluginResourcesProvider = Symbol.for('ResourcesProvider');
export interface PluginResourcesProvider {
Copy link
Member

@akosyakov akosyakov Sep 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource and ResourcProvider already have a meaning in Theia, could we come up with other names?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe PluginResourceContentProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akosyakov I think yes, but to me it is better to have name as close to the meaning as possible. I gave this name here because plugin resource is general word for images, scripts, styles, snippets etc. which a plugin has.
In addition to @AndrienkoAleksandr's proposal, I may suggest PluginDataProvider. Don't know which one is better.
WDYT?

Copy link
Member

@akosyakov akosyakov Sep 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be moved to the Che extension since the vanilla Theia does not seem to support remote extensions? It is hard to judge for me how it should be named and work since i cannot run it from examples.

I would be fine with doing something like https://github.com/theia-ide/theia/pull/6070/files#r319754377, i.e.

app.get('/hostedPlugin/:pluginId/:path(*)', async (req, res) => {
    // ...
    if (localPath) {
        // ...
    } else {
        this.handleMissingFile({pluginId, filePath, req, res});
    }
});

protected handleMissingResource(...): void {
    res.status(404).send(`The plugin with id '${escape_html(pluginId)}' does not exist.`);
}

in the che extension you subclass HostedPluginReader and override handleMissingFile, without any unused new APIs in the vanilla Theia

Alternatively it would be good to setup the browser example with remote extensions that any committer can test and reason about it from sources. If it is not feasible then it is not in the scope of the project. I generally think remote extensions is valid case, but in the current setup is hard to work on them. I've added a point to an agenda for tomorrow.

/**
* Checks if this provider has resources for the given plugin.
* @param pluginId id of plugin to check presence of resources for
*/
hasResources(pluginId: string): boolean;

/**
* Returns resource by given path or undefined if no resource exists.
* @param pluginId id of plugin that requests resource is for.
* @param resourcePath path to requested resource. Might be relative to plugin location
*/
getResource(pluginId: string, resourcePath: string): Promise<Buffer | undefined>;
mmorhun marked this conversation as resolved.
Show resolved Hide resolved
}

export function getPluginId(plugin: PluginPackage | PluginModel): string {
return `${plugin.publisher}_${plugin.name}`.replace(/\W/g, '_');
}
Expand Down
23 changes: 20 additions & 3 deletions packages/plugin-ext/src/hosted/node/plugin-reader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import * as escape_html from 'escape-html';
import { ILogger } from '@theia/core';
import { inject, injectable, optional, multiInject } from 'inversify';
import { BackendApplicationContribution } from '@theia/core/lib/node/backend-application';
import { PluginMetadata, getPluginId, MetadataProcessor } from '../../common/plugin-protocol';
import { PluginMetadata, getPluginId, MetadataProcessor, PluginResourcesProvider } from '../../common/plugin-protocol';
import { MetadataScanner } from './metadata-scanner';

@injectable()
Expand All @@ -36,15 +36,20 @@ export class HostedPluginReader implements BackendApplicationContribution {
private readonly scanner: MetadataScanner;

@optional()
@multiInject(MetadataProcessor) private readonly metadataProcessors: MetadataProcessor[];
@multiInject(MetadataProcessor)
private readonly metadataProcessors: MetadataProcessor[];

@optional()
@multiInject(PluginResourcesProvider)
Copy link
Member

@akosyakov akosyakov Sep 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never use multiInject, use ContributionProvider, that's the documented way to introduce an extension point; and the real type for multi inject is PluginResourcesProvider[] | undefined when there is no a single binding, see #4774 (comment) for more

I will update the coding guidelines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to the coding guidelines: https://github.com/theia-ide/theia/wiki/Coding-Guidelines#no-multi-inject

private readonly resourcesProvider: PluginResourcesProvider[];

/**
* Map between a plugin's id and the local storage
*/
private pluginsIdsFiles: Map<string, string> = new Map();

configure(app: express.Application): void {
app.get('/hostedPlugin/:pluginId/:path(*)', (req, res) => {
app.get('/hostedPlugin/:pluginId/:path(*)', async (req, res) => {
akosyakov marked this conversation as resolved.
Show resolved Hide resolved
const pluginId = req.params.pluginId;
const filePath = req.params.path;

Expand All @@ -54,6 +59,18 @@ export class HostedPluginReader implements BackendApplicationContribution {
res.status(404).send(`No such file for plugin with id '${escape_html(pluginId)}'.`);
mmorhun marked this conversation as resolved.
Show resolved Hide resolved
});
} else {
// Requested resource is not local. Check providers if any.
akosyakov marked this conversation as resolved.
Show resolved Hide resolved
for (const provider of this.resourcesProvider) {
if (provider.hasResources(pluginId)) {
const resource = await provider.getResource(pluginId, filePath);
if (resource) {
res.type(path.extname(filePath));
res.send(resource);
mmorhun marked this conversation as resolved.
Show resolved Hide resolved
return;
}
}
}

res.status(404).send(`The plugin with id '${escape_html(pluginId)}' does not exist.`);
}
});
Expand Down