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

upgrading to LSP 5.3.0 and Monaco 0.17.0 #5901

Merged
merged 2 commits into from
Sep 11, 2019
Merged

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Aug 9, 2019

What it does

How to test

  • test language features with native Theia extension:
    • test that features are available in the editor context menu and in quick command palette
    • a declaration is revealed when following the reference
    • focus belongs to a proper editor after following a reference (for internal and outside references, from editor to editor preview and view versa)
    • reference peek is still opened after following a reference (for internal and outside references, from editor to editor preview and view versa)
    • esc closes reference peek widget
    • completion items are properly resolved and inserted
    • focus belongs to a proper editor on peek definition (for internal and outside references, from editor to editor preview and view versa)
    • check that rename work inside the file and across files
    • check that arguments are proposed by the signature help
    • check that hover is present with properly rendered docs
    • check that symbol occurrences is highlighted when a declaration or reference is selected
    • check that go to (type|implementation) definition works properly, i.e. definition is revealed
    • check that code lens work properly
    • check that code actions work properly (quick fixes)
    • check that document and selection formatting work properly
    • check color provider with css colors
    • check link provider with links in html
    • check that folding ranges are properly provided for js, css and html
    • check go to symbol command
    • check open workspace symbol command
  • test language features with VS Code extension:
    • typesctipt VS Code extension for testing: ts_plugins.zip
    • to run it:
      • uninstall native typescript extension in example package.json
      • build the example app
      • unzip archive into plugins folder
      • start the example app
    • use the same checks as for native Theia extension
  • test quick palette:
    • proper keys are displayed for the quick command palette, i.e. they are displayed and a corresponding Monaco command can be triggered by such keybinding
    • use sample vscode extension
    • use fuzzy search
  • test vscode tree view (that context keys work properly)
  • test plugin editor decorations, for example with power mode extension
  • test debug console input editor
  • test debug breakpoint editor
  • test that preferences are propagated as config key values: change editor preferences and check whether they are reflected
  • test language auto detection
  • test edit menu with editor
  • test selection menu with editor
  • test editor context menu
  • smoke test some VS Code extensions like go, python, just use editor for a bit, check backend and frontend logs for errors

Review checklist

Reminder for reviewers

@akosyakov akosyakov added lsp language server protocol monaco issues related to monaco labels Aug 9, 2019
@akosyakov
Copy link
Member Author

@svenefftinge pointed out that babel won't work for plugin system :( I've opened #5902 to try to move es6 completely instead.

Alternatively, we can move PluginMessageReader and PluginMessageWriter to @theia/plugin-host package and transpile it to es6. Later we can put all code required for host plugin process in this package to make it self-contained for deployment. cc @theia-ide/plugin-system

@akosyakov akosyakov force-pushed the ak/upgrade_monaco branch 3 times, most recently from 1e1e82b to 1a932e8 Compare August 14, 2019 09:08
@akosyakov akosyakov added debug issues that related to debug functionality plug-in system issues related to the plug-in system labels Aug 14, 2019
@akosyakov akosyakov force-pushed the ak/upgrade_monaco branch 8 times, most recently from 0cc9e7b to 3c38d40 Compare August 15, 2019 15:04
@akosyakov akosyakov mentioned this pull request Aug 16, 2019
1 task
@akosyakov akosyakov force-pushed the ak/upgrade_monaco branch 5 times, most recently from 33d357f to 1ac5db0 Compare August 16, 2019 14:19
@akosyakov akosyakov added keybindings issues related to keybindings editor-preview issues that are related to the editor-preview labels Aug 17, 2019
@akosyakov akosyakov force-pushed the ak/upgrade_monaco branch 4 times, most recently from dd917df to d826df7 Compare August 18, 2019 12:52
@akosyakov
Copy link
Member Author

akosyakov commented Sep 6, 2019

I've managed to fix esc issue by registering all Monaco commands in our registry. We were doing it before but only for editor actions, not for global command like closeReferenceWidget. Now, we handle keybindings before Monaco and call closeReferenceWidget on esc keycode.

@akosyakov
Copy link
Member Author

akosyakov commented Sep 6, 2019

@marechal-p I've discussed scrolling issue with @svenefftinge, decided that is annoying but not critical, you can still use mouse. Hope they fix it soon and we migrate again. We need fresh Monaco to support latest LSP/VS Code apis, fix memory leaks and so on.

@akosyakov
Copy link
Member Author

akosyakov commented Sep 6, 2019

@tolusha Do you use some particular go project for testing? if i just create go file in theia repo nothing happens.

Was able to reproduce it, paths in editors decorations expected to be URIs now:
Screen Shot 2019-09-06 at 14 44 40

@akosyakov akosyakov force-pushed the ak/upgrade_monaco branch 2 times, most recently from fba1ab0 to 487face Compare September 6, 2019 09:19
@akosyakov
Copy link
Member Author

Got another error with Go:

root ERROR TypeError: Cannot add property source.organizeImports, object is not extensible
    at https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:14684:38
    at Array.forEach (<anonymous>)
    at mixin (https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:14672:33)
    at https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:14676:29
    at Array.forEach (<anonymous>)
    at Object.mixin (https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:14672:33)
    at Configuration.CommonEditorConfiguration.updateOptions (https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:81298:40)
    at StandaloneEditor.CodeEditorWidget.updateOptions (https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:103757:33)
    at StandaloneEditor.updateOptions (https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/vs/editor/editor.main.js:126146:44)
    at MonacoEditorProvider.updateMonacoEditorOptions (https://3000-c949f0b0-9946-4f3f-87b8-b600fb88e1c2.ws-eu0.gitpod.io/29.bundle.js:1302:33)

@akosyakov
Copy link
Member Author

@tolusha I think i was able to address all your issues.

@tolusha
Copy link
Contributor

tolusha commented Sep 6, 2019

@akosyakov
yeap, confirm

@akosyakov
Copy link
Member Author

@theia-ide/core @svenefftinge @marcdumais-work @tsmaeder Are we good to merge it? Or someone wants to test it more?

@akosyakov
Copy link
Member Author

akosyakov commented Sep 9, 2019

Found an error while changing and saving:

logger-protocol.ts:112 root ERROR Error: command 'monaco.editor.action.formatDocument' not found
    at StandaloneCommandService.executeCommand (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/vs/editor/editor.main.js:125588:39)
    at MonacoCommandService.<anonymous> (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:366:79)
    at step (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:299:23)
    at Object.next (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:280:53)
    at https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:274:71
    at new Promise (<anonymous>)
    at push.../../packages/monaco/lib/browser/monaco-command-service.js.__awaiter (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:270:12)
    at MonacoCommandService.push.../../packages/monaco/lib/browser/monaco-command-service.js.MonacoCommandService.executeCommand (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:357:16)
    at MonacoEditorProvider.<anonymous> (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:1328:68)
    at step (https://3000-d1c1c089-866a-49c2-a92a-7ce3a110b0cf.ws-eu0.gitpod.io/29.bundle.js:1092:23)

It is not a regression, format document is not supported for txt files and it was throwing an exception always.

@tsmaeder
Copy link
Contributor

tsmaeder commented Sep 9, 2019

I am all for merging this one: we'll need it to make progress on our goal to be fully VS Code API-compabible.
However, I simply don't have the capacity to do a proper code review (including testing) this week or next, so can't really approve it.
That said, I think due diligence has been done to make sure we're not seriously broken.

@svenefftinge
Copy link
Contributor

What about #5901 (comment) ?

LSP dependencies are targeting es6 now,
we are forced to use babel in order to transpile es6 to es5 classes for these dependencies

Signed-off-by: Anton Kosyakov <[email protected]>
Signed-off-by: Anton Kosyakov <[email protected]>
Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

Awesome work, Anton! 🎉 As always :-)

@kittaakos
Copy link
Contributor

Does this PR covers CreateFile | RenameFile | DeleteFile as a workspace edit? I do not see any support for this in the monaco workspace. Thanks!

CC: @akosyakov

@danidoedel
Copy link

@marechal-p The slow scrolling in Firefox should be fixed with Monaco 0.18.1
@akosyakov Are there any plans to upgrade in the near future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality editor-preview issues that are related to the editor-preview keybindings issues related to keybindings lsp language server protocol monaco issues related to monaco plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.