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

Don't use Monaco Concepts in Main/Ext Interface #4355

Open
tsmaeder opened this issue Feb 15, 2019 · 8 comments
Open

Don't use Monaco Concepts in Main/Ext Interface #4355

tsmaeder opened this issue Feb 15, 2019 · 8 comments
Assignees
Labels
design issues related to design enhancement issues that are enhancements to current functionality - nice to haves plug-in system issues related to the plug-in system

Comments

@tsmaeder
Copy link
Contributor

Right now, we are using Monaco conventions in the Main/Ext interface in the plugin-ext package. What I mean by this is that the shapes of interfaces used (See Range in model.ts, for example) are the same as in Monaco and positions are 1-based (instead of 0-based as in the Theia and VS Code plugin APIs).

I propose to move to a main/ext interface based on vscode-languageserver-types ("VLT"). Any conversion to monaco-specific types would be done on the main side of the main/ext interface. Here's why:

  1. Theia API is based on LSP types, for example, a "Problem" in Theia is defined as a Marker where the Diagnostic types is imported from VLT. So in the cases where the plugin API does not map to Monaco, we can just pass the types onto Theia API. Also, we already have a conversion infrastructure from VLT types to Monaco types in the monaco-languages client module. So we could get rid or a lot of conversion code.
  2. We could get rid of many definitions in model.ts and just import VLT, where needed
  3. This reason is architectural. Relying on Monaco conventions "exports" the knowledge that we're using Monaco to the plugin API. However, relying on Monaco (instead of the Theia editor API) is a bit of a code smell. As such, we would like to confine that fact to as small a place as possible.
@tsmaeder tsmaeder added plug-in system issues related to the plug-in system enhancement issues that are enhancements to current functionality - nice to haves design issues related to design labels Feb 15, 2019
@tsmaeder tsmaeder self-assigned this Mar 1, 2019
@tsmaeder
Copy link
Contributor Author

tsmaeder commented Mar 8, 2019

After investing a couple of days in this, I have been able to convert a lot of the main/ext protocol to use lsp types. However, I am not quite sure it makes sense to proceed anymore:

  1. Using types from a third-party project to transfer data over the wire is not really safe: just like with the classes from the theia API implementation (types-impl.ts), they may use accessors and as such may not marshal to the form we expect (think Position { _line: number, ...). The only way to safely transfer is to create the objects ourselves: this means not using the "create" functions from the languageserver-types module
  2. The VS Code version used in monaco-languageclient is relatively old. It does not have newer capabilities like file-level changes in workspace edits. The problem with this is that the vscode-languageclient package used in VS Code extensions will advertise those capabilities. This means the type conversion functions from monaco-languagclient need to be inspected for compatibility or we need to keep monaco-languageclient up-to date with the VS Code API we support.
  3. There are no LSP features in the the Theia editor anyway: they are all tightly bound to Monaco anyway, so we have to do type conversion for every call on the main side of the main/ext interface.

@akosyakov
Copy link
Member

I don't think we need to stick purely to LSP types, but getting rid of Monaco dependencies from the plugin host process would be good.

@akosyakov
Copy link
Member

A bit off-topic: for Gitpod, we (TypeFox) would like to spawn the plugin host process in a user side-container, i.e where user filesystem, processes, terminals and tools live, and ssh into it. Obviously we don't want to install the whole Theia because of it in this container. It would be good to extract the code for the plugin host process in its own npm package. It should be self-contained and not use code from any Theia extensions, so by doing it dependencies to Monaco should be eliminated. @theia/plugin-ext can use it. cc @svenefftinge

cc @caseyflynn-google maybe you are interested in such Arch as well, not sure how you are using side-containers and whether your goal is to run VS Code extensions as close as possible to VS Code.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Mar 8, 2019

I've pushed my current state to https://github.com/tsmaeder/theia/tree/4355_use_lsp_types

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Mar 8, 2019

It would be good to extract the code for the plugin host process in its own npm package. It should be self-contained and not use code from any Theia extensions, so by doing it dependencies to Monaco should be eliminated. @theia/plugin-ext can use it. cc @svenefftinge

So separate the ext side of the main/ext interface. Yes, I like it. To be precise, the ext side of the interface right now does not technically depend on monaco implementation, but it uses interfaces that are mostly compatible with monaco objects.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Mar 8, 2019

For the completions, for example, the fact that we're using Monaco-compatible types, allows us to tunnel and "ident" attribute through the provide/resolve cycle: we rely on the fact that when we provide e.g. a code lens, to monaco, we will get the same object back when we are asked to resolve it. If we do some conversion, we would have explicitly remember the ident field when converting.

@benoitf
Copy link
Contributor

benoitf commented Mar 11, 2019

@tsmaeder what is remaining before opening PR from your branch ?

@tsmaeder
Copy link
Contributor Author

I'd like to discuss if it even makes sense to proceed with this at the current time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design issues related to design enhancement issues that are enhancements to current functionality - nice to haves plug-in system issues related to the plug-in system
Projects
None yet
Development

No branches or pull requests

3 participants