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

hook monaco cancellation into plugin language services #4914

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Apr 15, 2019

fix #4909

Monaco cancels requests on new user input to improve responsiveness. This PR makes sure that the plugin system respects Monaco cancellations.

I've tested with go and rust vscode extensions. If someone test it with some other languages it would be nice.

@@ -26,6 +26,7 @@ import { Event } from '@theia/core/lib/common/event';
import { Deferred } from '@theia/core/lib/common/promise-util';
import VSCodeURI from 'vscode-uri';
import URI from '@theia/core/lib/common/uri';
import { CancellationToken, CancellationTokenSource } from 'vscode-jsonrpc/lib/cancellation';
Copy link
Contributor

Choose a reason for hiding this comment

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

it's kind of minor but shouldn't we import from vscode-languageserver-protocol instead of vscode-jsonrpc (I know it's defined in vscode-jsonrpc but it's odd here to depends from jsonrpc instead of the protocol vscode-languageserver-protocol

https://github.com/Microsoft/vscode-languageserver-node/blob/dbe64946d7e41a50996a7c2929ac4aa4911b505d/protocol/src/main.ts#L21

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
Member Author

Choose a reason for hiding this comment

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

I want to remove dependencies to theia extensions from plugin host process as much as possible. Will replace with import from protocol

Copy link
Contributor

Choose a reason for hiding this comment

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

remove dependencies to theia extensions

it includes @theia/core as well ?

Copy link
Member Author

Choose a reason for hiding this comment

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

An idea was to allow spawning plugin host process over ssh in another container. @theia/core should not be necessary then to install at build time and load at runtime. It's another issue though I can import from @theia/core as well for now if you like it more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine to limit dependencies (it's good). But I'm still wondering which package need to be 'cut off'
Like reducing core dependencies, or reducing external dependencies vscode-* dependencies ?

I think the goal is to have the smallest bootstrap but I don't know which path is better

I can import from @theia/core as well for now if you like it more.

I'm fine with any dependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

@theia/core has quite a lot of unrelated code and pulls difference dependencies like phosphor. I would prefer not installing it.

In Che as far as I understood you deploy Theia in each side container to serve a VS Code extension. I guess it would be interesting as well just deploy code only for host process and wrap it in node server without @theia/core.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akosyakov yes I share the same concern, I just wanted to be sure. (for me there is phosphor but also electron right now) and yes many unwanted stuff. I don't know when to start the split of plugin-ext module as it's highly changed right now

Copy link
Member Author

Choose a reason for hiding this comment

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

We've removed all electron deps btw:
#4873
#4882
#4885
theia-ide/theia-apps#165

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks, I'm still reading mails/changes after one week of PTO :)

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov force-pushed the ak/plugin_lang_cancel branch from 8c5ae49 to c58678e Compare April 15, 2019 11:49
@akosyakov
Copy link
Member Author

Does anyone going to test a bit? Or no objections to merge?

@benoitf
Copy link
Contributor

benoitf commented Apr 16, 2019

Merge

@akosyakov akosyakov merged commit 4286ebd into master Apr 16, 2019
@akosyakov akosyakov deleted the ak/plugin_lang_cancel branch April 16, 2019 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] support cancellation
2 participants