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

Theia plugin API theia.commands.registerCommand is incompatible with VSCode #7295

Closed
JonasHelming opened this issue Mar 8, 2020 · 11 comments · Fixed by #7296
Closed

Theia plugin API theia.commands.registerCommand is incompatible with VSCode #7295

JonasHelming opened this issue Mar 8, 2020 · 11 comments · Fixed by #7296
Labels
plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility

Comments

@JonasHelming
Copy link
Contributor

JonasHelming commented Mar 8, 2020

Description

As stated here, the Theia plugin API should be a strict super set of the VSCode extension API. theia.commands.registerCommand does only allow to register (CommandsDescription, handler), but not (commandID: string, handler) as VSCode does.

Reproduction Steps

The first registration below does not compile, the other two do.

import * as theia from '@theia/plugin';
import * as vscode from 'vscode';

export function start(context: theia.PluginContext) {
    const informationMessageTestCommand = {
        id: 'hello-world-example-generated2',
        label: "Hello World2"
    };
    context.subscriptions.push(theia.commands.registerCommand('hello-world-example-generated1', (...args: any[]) => {
        theia.window.showInformationMessage('Hello World1!');
    }));

    context.subscriptions.push(theia.commands.registerCommand(informationMessageTestCommand, (...args: any[]) => {
        theia.window.showInformationMessage('Hello World2!');
    }));

    context.subscriptions.push(vscode.commands.registerCommand('hello-world-example-generated3', (...args: any[]) => {
        theia.window.showInformationMessage('Hello World3!');
    }));

}

OS and Theia version:
latest

JonasHelming added a commit to eclipsesource/theia that referenced this issue Mar 8, 2020
 fixed theia.commands.registerCommand to be compatible with VSCode

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
@JonasHelming
Copy link
Contributor Author

proposed solution, see PR #7296

@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Mar 8, 2020
JonasHelming added a commit to eclipsesource/theia that referenced this issue Mar 8, 2020
….commands.registerCommand to be compatible with VSCodeSigned-off-by: Jonas Helming <jhelming@eclipsesource.com>

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Mar 9, 2020
@akosyakov
Copy link
Member

akosyakov commented Mar 9, 2020

@JonasHelming I wonder whether it is really important for you to use @theia/plugin namespace. I would just go with vscode. I've mentioned already in other threads that the current approach is troublesome: it is hard to know when @theia/plugin namespace is covered by vscode and when not. Even if we achieve the complete super set the problem will remain. Clean separation of namespaces would be better as proposed on the spectrum thread, then you can do runtime checks to decide which API can be used or not.

@benoitf
Copy link
Contributor

benoitf commented Mar 9, 2020

I was on PTO last week so I haven't commented but I think we said that idea was to use @theia/plugin namespace to use APIs that are not part of VS Code API. (so no superset)
So for example to register commands, it would done using VS Code API.

But let say if you want to register custom namespace then you'll just import it.
No need to do 'acquireApi'

for example

import * as vscode from 'vscode'
import * as theia from '@theia/plugin'
import * as che from '@eclipse-che/plugin'


...
vscode.commands.registerCommand(...)
theia.env.getClientOperatingSystem()
che.workspace.getCurrentWorkspace()

@akosyakov
Copy link
Member

akosyakov commented Mar 9, 2020

How do you check at runtime whether @theia/plugin is not available for VS Code? try - catch for each statement?

With acquire API typescript fails at the compilation time, plus one does not need to hook into the plugin system anymore to add a new namespace. namespace API is built based on generic commands wrapped in an API object. It could allow to move in the direction of #6353 (comment) to make support of VS Code extensions easier but at the same time support custom namespaces on the top.

(fyi) I'm not going to do anything about it :) I'm just entertaining an idea of how to make our maintenance effort easier.

@JonasHelming
Copy link
Contributor Author

@benoitf :
"I think we said that idea was to use @theia/plugin namespace to use APIs that are not part of VS Code API. (so no superset)"
Just to make sure that we are on the same page here:
I believe the Theia plugin API is currently a super set of VSCode (except this issue). So do you propose a change to this paradigm? I think #6353 (comment) describes exactly this idea, i.e. the Theia API ONLY contains what is not supported in VSCode.

@akosyakov
Copy link
Member

@JonasHelming there are more differences, you can see them plugin-vscode-init.ts, i.e. theia.plugins vs vscode.extensions namespace.

@benoitf
Copy link
Contributor

benoitf commented Mar 9, 2020

@JonasHelming yes it's a shift of paradigm.
But proposing almost the same API with a different namespace is not very well understood.
It was only nice at the beginning when starting to implement API.

@JonasHelming
Copy link
Contributor Author

JonasHelming commented Mar 9, 2020

@akosyakov : Yes, found that, I believe my PR removes one of those differences. It is probably the one that most users will immediatly notice, as it breaks every simple "hello world" example.
I had a quick look at the otther adaptations, they do not look like incompatilities to me, though.

@benoitf : OK, thank you for the clarification, that makes a lot of sense! However, if that is decided, wouldn't it make sense to NOT advocate the use of the theia namespace for plugin developers atm, but rather the use of the vscode namespace?

@akosyakov
Copy link
Member

akosyakov commented Mar 9, 2020

However, it that is decided

It has to be discussed with @svenefftinge and @marcdumais-work. The initial idea, at least from our side, was to gain credibility by being compatible with VS Code extensions and we liked very much that RedHat team took this concern and aligned theia.d.ts with vscode.d.ts. We gain a lot by relying on any improvements done by VS Code team, and diverging will for sure harm it as well as Theia participation in the VS Code ecosystem. If VS Code won't have a leverage like many end users and extensions or Theia has many, I think we could discuss a different approach. But in the current situation embracing VS Code and letting to extend it is the best choice.

@JonasHelming
Copy link
Contributor Author

@svenefftinge @marcdumais-work any thoughts on this?
I think there are three main questions:

  1. Do you want to get the current fix? I believe it is valuable, as it removes a confusing diff between the API right away
  2. Do we stop advocating the usage of the Theia namespace and recommend using vscode (I am happy to contribute to that)
  3. Do we mid term deprectae the theia namespace or at least change its purpose from being a clone of vscode

@marcdumais-work
Copy link
Contributor

Hi @JonasHelming ,

Do you want to get the current fix? I believe it is valuable, as it removes a confusing diff between the API right away

This one seems relatively simple to decide. If I understand correctly, the proposed change does not break backward compatibility and, at least until we officially decide where to go with the namespaces, it will likely be useful to plugin developers that are familiar with the VS Code API and might be annoyed by this small incompatibility, while almost everything else is identical.

So, IMHO, there is no downside accepting this small contribution, other than the fact that it may not matter for very long, once we proceed with something like #6353 (comment) . Am I missing something?

JonasHelming added a commit to eclipsesource/theia that referenced this issue Mar 26, 2020
…VSCode eclipse-theia#7295

fixed theia.commands.registerCommand to be compatible with VSCode

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
JonasHelming added a commit to eclipsesource/theia that referenced this issue Mar 26, 2020
…theia#7295

fixed theia.commands.registerCommand to be compatible with VSCode

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
vince-fugnitto pushed a commit that referenced this issue May 8, 2020
fixed theia.commands.registerCommand to be compatible with VSCode

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
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 vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants