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.

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

Add plugins resources provider contribution point #6070

Closed
wants to merge 1 commit into from
Closed

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Aug 30, 2019

What it does

This PR adds contribution point into plugins resources endpoint. This allows advanced resources handling in case if resources of a plugin are located in another place or should be generated on the go.

How to test

Move a plugin resources into another folder and add simple implementation of PluginResourcesProvider contribution which just returns the resources.

Review checklist

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@tsmaeder
Copy link
Contributor

@mmorhun can we have a link to an issue, please?

@mmorhun
Copy link
Contributor Author

mmorhun commented Aug 30, 2019

@tsmaeder this PR is needed for resolving of eclipse-che/che#13750

private readonly metadataProcessors: MetadataProcessor[];

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

@akosyakov akosyakov Sep 1, 2019

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.

* 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

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

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.

@akosyakov
Copy link
Member

akosyakov commented Sep 1, 2019

It would be good to have the remote example in this repo without any external service. Otherwise we collect dead APIs without internal clients, it is not clear how to test and reason about their usefulness and future changes to it.

As an alternative, instead of new APIs a new method could be extracted and overiden by a concrete product, or next express.js could be called: https://expressjs.com/en/guide/using-middleware.html#middleware.application

Although, I think that remote extensions are valid case and we should have better setup that one does not verify APIs with downstream projects.

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Sep 1, 2019
@akosyakov
Copy link
Member

It was discussed that new abstractions should be backend up with at lest 2 solid (relevant to now, not imaginary in future) Theia specific use-cases. Clients for a new abstraction should be provided in Theia repo to track breaking changes, it can be an example extension. It is done to avoid overgeneralizing framework with unused abstractions and APIs based on needs of a single product. For a single case, customizations have to be done to this product. Framework can be changed though to simplify customization of a product, meaning extracting methods, classes and so on.

For this PR, a new template method can be extracted as proposed in #6070 (comment) (the minimal change) or the whole logic can be moved to something like PluginResourceRequestHandler.handle(request, response) in order to have an explicit API.

@mmorhun
Copy link
Contributor Author

mmorhun commented Sep 6, 2019

Closing in favour of #6126

@mmorhun mmorhun closed this Sep 6, 2019
@mmorhun mmorhun deleted the CHE-13750 branch September 6, 2019 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants