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

[plugin][languages]Add Plugin API for language server contribution #3805

Merged
merged 1 commit into from
Dec 18, 2018
Merged

[plugin][languages]Add Plugin API for language server contribution #3805

merged 1 commit into from
Dec 18, 2018

Conversation

svor
Copy link
Contributor

@svor svor commented Dec 11, 2018

Signed-off-by: Valeriy Svydenko vsvydenk@redhat.com

This PR provides simple Plugin API as proposed which allows to register new Language Server from the plug-in. For this was added new namespace:

    export namespace languageServer {
        /**
         * Registers new language server.
         *
         * @param languageServerInfo information about the language server
         */
        export function registerLanguageServerProvider(languageServerInfo: LanguageServerInfo): void;

        /**
         * Stops the language server.
         *
         * @param id language server's id
         */
        export function stop(id: string): void;
    }

To register new Language Server a plugin has to call
registerLanguageServerContribution(languageContribution: PluginLanguageContribution)
where languageContribution provides an information about LS contribution:

    export interface PluginLanguageContribution {
        /**
         * Language server's id.
         */
        id: string;
        /**
         * Language server's name.
         */
        name: string;
        /**
         * The command to run language server as a process.
         */
        command: string;
        /**
         * Command's arguments.
         */
        args: string[];
        /**
         * File's patterns which can be used to create file system watchers.
         */
        globPatterns?: string[];
        /**
         * Names of files. If the workspace contains some of them language server should be activated.
         */
        workspaceContains?: string[];
    }

To stop the language server need to call languageServer.stop(languageServerId)

registerLanguageClientContribution(clientContribution: LanguageClientContribution): Disposable {
const id = clientContribution.id;
if (this.isRegistered(id)) {
console.warn(`Language contribution with type '${id}' already registered.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, use of Logger ?

}

private isRegistered(languageId: string): boolean {
for (const id of this.languageClientContributors.keys()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(maybe then we don't need a method for that)

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2018

Adding review of @akosyakov as packages/languages is modified

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2018

@svor here are some questions

  • why it's a contribution and not a provider. Almost all APIs are using the keyword provider
  • namespace is not in languages or a sub module ? why do we need languagesContribution
  • I don't see any stop ? (or callback on stop)
  • why command is a string and not like an array of parameters as it may be be an issue to know if args have space in them or not
  • why globPatterns for filesystem watchers as there is already an API to ask for filewatchers ?
  • what is An array of files' names ?

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2018

also you may discuss with @evidolob but AFAIK it should go on the "proposed" scope as the API might change after ppl start to play with it

@svor
Copy link
Contributor Author

svor commented Dec 12, 2018

About "proposed" scope. I'm going to create theia-proposed.d.ts and import this file into theia.d.ts. After that I'll move this new API into theia-proposed.d.ts.
@evidolob @benoitf are you OK with it?

@akosyakov
Copy link
Member

akosyakov commented Dec 12, 2018

MonacoLanguageClient is implemented by shimming relevant VS Code APIs for LanguageClient from vscode-languageclient: https://github.com/TypeFox/monaco-languageclient/blob/master/src/vscode-api.ts. In order to maintain, we have to recompile vscode-languageclient to es5 and each time when a new version is released check that all relevant APIs are covered. Last time it took a week.

With the plugin system in place, MonacoLanguageClient can be deprecated as well as LanguageClientContribution and LanguageServerContribution. Instead of maintaining it, we can help with maintaining compatibility with VS Code, and use directly vscode-languageclient in the Theia plugins/VS Code extensions. As far as I understood, it is possible to use vscode and theia namespace next to each other.

I don't really see a point of exposing in Theia plugin API something what should be deprecated eventually.

//cc @svenefftinge

@svor
Copy link
Contributor Author

svor commented Dec 13, 2018

@benoitf

why it's a contribution and not a provider. Almost all APIs are using the keyword provider
namespace is not in languages or a sub module ? why do we need languagesContribution

I've made some changes, now the namespace looks like:

   export namespace languageServer {
        /**
         * Registers new language server.
         *
         * @param languageServerInfo information about the language server
         */
        export function registerLanguageServerProvider(languageServerInfo: LanguageServerInfo): void;

        /**
         * Stops the language server.
         *
         * @param id language server's id
         */
        export function stop(id: string): void;
    }

and this namespace was added as proposed into theia-proposed.d.ts.

I don't see any stop ? (or callback on stop)

I didn't think how the new Language Server should be stopped or unregistered, so for now I've added one more method into API to make it possible.

why command is a string and not like an array of parameters as it may be be an issue to know if args have space in them or not

I have another field to describe parameters or arguments for the command:

/**
 * Command's arguments.
 */
args: string[];

or i misunderstood your question?

why globPatterns for filesystem watchers as there is already an API to ask for filewatchers ?
what is An array of files' names ?

PluginLanguageClientContribution extends BaseLanguageClientContribution which has
method which provides patterns to register file system watchers
and a method which provides names of files to check if some file exists in the workspace - LS should be activated (for example pom.xml for Java LS). So that's why API should have this information

To check this API I've created a plugin which is based on XML language server and I'm going to provide a PR into theia-samples with this plugin when current PR will be resolved.

@svor svor added the Team: Che-Languages issues regarding the che-languages team label Dec 13, 2018
@benoitf
Copy link
Contributor

benoitf commented Dec 13, 2018

@svor

export function registerLanguageServerProvider(languageServerInfo: LanguageServerInfo): void;

should return a Disposable object ?

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

I'm fine for plug-in/ext changes as it goes on a proposed.d.ts, need approval of @akosyakov for languages changes

@svenefftinge
Copy link
Contributor

The comment from @akosyakov really should be addressed before merge. Please ask if it is unclear.

@tsmaeder
Copy link
Contributor

tsmaeder commented Dec 14, 2018

I don't really see a point of exposing in Theia plugin API something what should be deprecated eventually.

//cc @svenefftinge

@akosyakov the extension point this PR introduces is for contributing language servers. The fact that it is implemented using MonacoLanguageClient is not not exposed in the API and therefore not relevant to the discussion
Currently, theia plugins do not have the vscode namespace available to them, and as such, we cannot use vscode-lanaguageclient. That's why we decided to go with this low-effort approach instead of forking or rewriting vscode-languageclient.
The real disscussion we need to have is whether the vscode API is a help to reuse existing plugins in some cases or whether it is our main and only way to write plugins. There are a number of folks involved with the plugin system that are against making the vscode API the "official" API, I guess mostly for strategic reasons (we don't have any control over what happens with the VSCode extension API).

I'll let @benoitf take over this discussion, though, since I'll be on vacation starting tonight.

@akosyakov
Copy link
Member

I'm on vacation starting today as well.

The real disscussion we need to have is whether the vscode API is a help to reuse existing plugins in some cases or whether it is our main and only way to write plugins.

Agreed, such strategic decisions should be discussed/decided by Theia community.

There are a number of folks involved with the plugin system that are against making the vscode API the "official" API, I guess mostly for strategic reasons.

For Theia project, ability to run VS Code extensions in Theia, and even vice versa, to run Theia plugins in VS Code, is important. It makes easier for (VS Code extension) developers to use Theia API namespace without fear to be locked-in and invest time in the platform that does not have yet big user base in comparison to VS Code. Having interchangeable vscode and theia APIs next to each other would attract such devs. I don't understand why we would create artificial barriers for devs.

we don't have any control over what happens with the VSCode extension API

We don't have any control over Monaco, LSP and DAP as well. In order to contribute to LSP, one should provide the implementation for one of the clients. Even with one implementation, one should still wait when VS Code team align proposed features with vscode APIs. By allowing and aligning Theia and VS Code APIs, we make it easier for VS Code team and it would be less work for us to align with them.

the extension point this PR introduces is for contributing language servers. The fact that it is implemented using MonacoLanguageClient is not not exposed in the API and therefore not relevant to the discussion

We should not have several ways of doing the same thing in the framework. If using and collaborating on vscode-languageclient and vscode API namespace is a way to go for Theia, then this PR is irrelevant.

@benoitf
Copy link
Contributor

benoitf commented Dec 17, 2018

There are a number of folks involved with the plugin system that are against making the vscode API the "official" API, I guess mostly for strategic reasons.

For Theia project, ability to run VS Code extensions in Theia, and even vice versa, to run Theia plugins in VS Code, is important. It makes easier for (VS Code extension) developers to use Theia API namespace without fear to be locked-in and invest time in the platform that does not have yet big user base in comparison to VS Code. Having interchangeable vscode and theia APIs next to each other would attract such devs. I don't understand why we would create artificial barriers for devs.

We're not creating artificial barrier. Also the initial plan was described 6 month ago there #1482 so no surprise here.

we don't have any control over what happens with the VSCode extension API

We don't have any control over Monaco, LSP and DAP as well. In order to contribute to LSP, one should provide the implementation for one of the clients. Even with one implementation, one should still wait when VS Code team align proposed features with vscode APIs. By allowing and aligning Theia and VS Code APIs, we make it easier for VS Code team and it would be less work for us to align with them.

It would limit the innovation. Also some usecase may be valid only for "online" IDE vs a Desktop IDE. So better to have compliance but still permit other ways required by other behaviors.
Also in all specifications, there are always specific API in addition, this is where product may differentiate as well.

the extension point this PR introduces is for contributing language servers. The fact that it is implemented using MonacoLanguageClient is not not exposed in the API and therefore not relevant to the discussion

We should not have several ways of doing the same thing in the framework. If using and collaborating on vscode-languageclient and vscode API namespace is a way to go for Theia, then this PR is irrelevant.

Disagreeing. Framework is open and should leverage innovation, not being blocked-up on waiting VSCode decide to do something. And wait again before being able to use it. Compliance doesn't imply to be default. AFAIK Theia is a framework, not a product.

Based on the previous feedback, let see how to modify only plug-in extension and not languages package. As it's a proposal API, it can be tested, changed, updated and even removed if it goes really wrong.

@svor
Copy link
Contributor Author

svor commented Dec 18, 2018

@benoitf now all changes are located in plugin-ext. Can you please take a look

@svor svor requested a review from tolusha December 18, 2018 13:50
@benoitf
Copy link
Contributor

benoitf commented Dec 18, 2018

@svor FYI branch has some conflicts

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>
@benoitf
Copy link
Contributor

benoitf commented Dec 18, 2018

LGTM now, all stuff is only related to plug-ins

@svor svor merged commit 36f22db into eclipse-theia:master Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team: Che-Languages issues regarding the che-languages team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants