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

fix #7100: remove @theia/languages and monaco-languageclient #8131

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Jul 2, 2020

What it does

  • fix remove dependency to monaco-languageclient #7100: remove @theia/languages and monaco-languageclient - It allows to consume Monaco directly without trying to run vscode-languageclient in the browser. One should contribute language smartness via VS Code extensions instead.
  • It cannot be part of 1.4.0, at least requires 1.5.0

TODO:

How to test

  • workspace symbols provided by VS Code extensions should be available:
    • wait till ts is initialized after opening a file
    • trigger Go to Workspace Symbol... command
    • type some prefix, wait till symbols are found
    • select some symbol and navigate to it
  • suggestion in the debug console should be properly provided and applied
    • put a breakpoint in uri.spec.ts
    • debug with Run Mocha Test
    • use the debug console to lookup variables
    • use different prefixes, apply suggestions and check that they resolve to proper variable names
  • language status bar entry should work
    • open some file
    • try to change a language
    • try to auto detect
  • git blame hover should work
    • open some file
    • trigger blame annotations
    • hover over them and check that hover corresponds to commit in the ruler
  • markdown preview widget should be in sync with text editors as before
    • open several markdown files and preview them (make sure to use preview from Theia, not built-in extension, one with old icon)
    • check that scrolling between preview and editor in sync
    • check that dirty changes are reflected
    • check that closed dirty editor without save reverts dirty changes in the preview
  • call hierarchies contributed by VS Code extensions should work
    • open ts file and wait till ts server is ready
    • trigger Open Call Hierarchy on some method
  • problem markers should be properly reflected
    • open some ts file
    • break it, should see markers in the problems view
    • delete it, markers should be removed
    • revert it in the scm view, markers should be added back
    • revert the dirty changes, markers should be removed

Review checklist

Reminder for reviewers

@akosyakov akosyakov added monaco issues related to monaco vscode issues related to VSCode compatibility languages issue related to languages labels Jul 2, 2020
@akosyakov
Copy link
Member Author

@tsmaeder @benoitf How do we deal with languageServer namespace? We don't want to add a dependency to a concrete version of vscode-languageserver in the plugin system. Should it be the same use VS Code extension instead? Do you use it in practice?

@tsmaeder
Copy link
Contributor

tsmaeder commented Jul 5, 2020

@akosyakov I'm not aware of anyone using the languageserver namespace. AFAIC go ahead and kill it.

@akosyakov akosyakov force-pushed the ak/drop_monaco_language_client branch from a2a0663 to d3b5a5a Compare July 5, 2020 12:29
akosyakov added a commit that referenced this pull request Jul 5, 2020
These APIs will be removed in next release via #8131

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov marked this pull request as ready for review July 5, 2020 13:19
@akosyakov
Copy link
Member Author

It's ready, but I don't think we need to review it yet. More important now to help with landing #8010 and fixing #7608 in this month. Otherwise everything will be postponed for one month.

documentChanges: true
}
};
export class MonacoWorkspace {

protected resolveReady: () => void;
readonly ready = new Promise<void>(resolve => {
Copy link
Member Author

Choose a reason for hiding this comment

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

it should be removed

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

Tested according to How to test section.
Used GitLens extension for checking Git blame annotations.
Everything works as expected.

akosyakov added a commit that referenced this pull request Jul 8, 2020
These APIs will be removed in next release via #8131

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@benoitf benoitf mentioned this pull request Jul 30, 2020
47 tasks
@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jul 30, 2020

I rebased and fixed conflicts for the PR as was discussed #8010 (comment)

There is copy branch without rebasing just in case https://github.com/eclipse-theia/theia/tree/ak/drop_monaco_language_client_copy

I'm going to wait for ci build/tests and provide light manual testing tomorrow morning before merge the PR.

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

I did light testing after rebasing:

  • workspace symbols
  • debug console
  • git blame hover
  • call hierarchies
  • problem markers

It works well for me - I don't see a regression.

So, I'm going to merge the PR and then adapt #8010 to these changes.

@kittaakos
Copy link
Contributor

So, I'm going to merge the PR

Can we wait a few more days, please?

@benoitf
Copy link
Contributor

benoitf commented Jul 31, 2020

hi @kittaakos, could you explain why you need more days ?

We just have a fresh tagged release so I think it's ok to merge all major changes in repo after this tag

like drop monaco-language-client, update FS api, bump monaco, etc.

@kittaakos
Copy link
Contributor

hi @kittaakos, could you explain why you need more days ?

All downstream projects still using language contributions from Theia cannot use the next version anymore.

@benoitf
Copy link
Contributor

benoitf commented Jul 31, 2020

@kittaakos I'm not sure few more days would help then ? as basically at some point next will still remove the support of monaco-language-client as target is for 1.5.0
I mean, if it's dropped on monday, it will still break on monday.

@kittaakos
Copy link
Contributor

I mean, if it's dropped on monday, it will still break on monday.

I still have time to code until Monday.

@benoitf
Copy link
Contributor

benoitf commented Jul 31, 2020

ok so let's wait until monday @RomanNikitenko ?

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jul 31, 2020

I would like to land #8010 ASAP
but, It's OK for me to wait until Monday if it helps to resolve the problem for downstream projects

@kittaakos
Copy link
Contributor

to resolve the problem for downstream projects

Yes, it helps. For example, here is a Theia issue: #8281
The only way to pick up such a fix by downstreams is by keeping the Theia version on next.

@benoitf
Copy link
Contributor

benoitf commented Jul 31, 2020

@kittaakos we could do some 1.4 bugfixes release as well (we did some of them in the past)

@kittaakos
Copy link
Contributor

I would like to land #8010 ASAP
but, It's OK for me to wait until Monday

@RomanNikitenko, go for it. Thanks for your patience.

It allows to consume Monaco directly without trying to run vscode-languageclient in the browser. One should contributed language smartness via VS Code extensions instead.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov merged commit 1e09b81 into master Aug 3, 2020
@akosyakov akosyakov deleted the ak/drop_monaco_language_client branch August 3, 2020 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
languages issue related to languages monaco issues related to monaco vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove dependency to monaco-languageclient
6 participants