-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun can we have a link to an issue, please? |
@tsmaeder this PR is needed for resolving of eclipse-che/che#13750 |
private readonly metadataProcessors: MetadataProcessor[]; | ||
|
||
@optional() | ||
@multiInject(PluginResourcesProvider) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
* Designed to retreive plugins resources from non default or remote location. | ||
*/ | ||
export const PluginResourcesProvider = Symbol.for('ResourcesProvider'); | ||
export interface PluginResourcesProvider { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe PluginResourceContentProvider?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 Although, I think that remote extensions are valid case and we should have better setup that one does not verify APIs with downstream projects. |
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 |
Closing in favour of #6126 |
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