-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Upgrade Monaco dependency to 1.83.1 #13217
Upgrade Monaco dependency to 1.83.1 #13217
Conversation
Note that the failure in test are due to the way nodejs handle ES6 modules: here's what I fount out:
This is the only place actual
This is because typescript compiles import to use "require". So we can't really mix & match ESM and CJS modules, it seems. |
Maybe we can hack the module in question to remove the exports, but what about the next time some ES6-syntax appears in VS Code? |
d99e532
to
d4198cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks generally fine, but I have a few remarks.
Most importantly, the performance of the editor initialization/syntax highlighting seemed to have noticably suffered:
Current master
:
2024-02-01.13-37-15.mp4
Changes:
2024-02-01.13-37-03.mp4
Can you reproduce the performance regression? I've had a feeling for quite a while now that our monaco editors are way slower to initialize than vscode - something that was actually been reinforced by the bad performance of the notebook editors. However, with the new changes it's pretty jarring.
Additionally, you might not be aware of this file. The command in there is supposed to be executed to align the Theia editor preferences to the preferences in Monaco/vscode. Furthermore there are quite a few lines/methods annotated with @monaco-uplift
that should be easily resolved with just 1-2 lines of changes.
@@ -173,9 +174,11 @@ const codeIconMap: Record<string, string> = { | |||
'remote-explorer-view-icon': 'remote-explorer', | |||
'review-comment-collapse': 'chevron-up', | |||
'run-view-icon': 'debug-alt', | |||
'runtime-extensions-editor-label-icon': ' extensions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is the space here intended?
'runtime-extensions-editor-label-icon': ' extensions', | |
'runtime-extensions-editor-label-icon': 'extensions', |
@@ -229,24 +233,14 @@ const codeIconMap: Record<string, string> = { | |||
'watch-expressions-add-function-breakpoint': 'add', | |||
'watch-expressions-remove-all': 'close-all', | |||
'watch-view-icon': 'debug-alt', | |||
'widget-close': 'close' | |||
'widget-close': 'close', | |||
'workspace-trust-editor-label-icon': ' shield' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'workspace-trust-editor-label-icon': ' shield' | |
'workspace-trust-editor-label-icon': 'shield' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msujew the performance hit vs. VS Code seems to be mostly due to the patched |
@msujew I believe the performance problem should be fixed with the latest commit. Can you give it a go? |
5473f5d
to
020bc5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me. I couldn't find any further regressions and the performance is back to the before-change level.
Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
- Register vscode icons in registry instead of overriding ThemeIcon ids to codicon ids - Use Monaco icon registry instead of adding our own icon stylying Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
0df996f
to
26d4d97
Compare
Before this fix, Monaco API KeybindingService.resolveKeybinding test was failing on macOS with ``` AssertionError: expected { label: '⌃⇧⌥⌘K', …(6) } to deeply equal { label: '⌃⇧⌥⌘K', …(7) } + expected - actual { "WYSIWYG": true "ariaLabel": "⌃⇧⌥⌘K" + "chord": false "dispatchParts": [ "ctrl+shift+alt+meta+K" ] "electronAccelerator": "Ctrl+Shift+Alt+Cmd+K" ``` The `"chord": false` line was removed as part of eclipse-theia#13217 for other platforms. This fix removes it for macOS.
Before this fix, Monaco API KeybindingService.resolveKeybinding test was failing on macOS with ``` AssertionError: expected { label: '⌃⇧⌥⌘K', …(6) } to deeply equal { label: '⌃⇧⌥⌘K', …(7) } + expected - actual { "WYSIWYG": true "ariaLabel": "⌃⇧⌥⌘K" + "chord": false "dispatchParts": [ "ctrl+shift+alt+meta+K" ] "electronAccelerator": "Ctrl+Shift+Alt+Cmd+K" ``` The `chord: false` was removed from the expected object for other platforms as part of eclipse-theia#13217. This fix removes it from the expected object for macOS.
Before this fix, Monaco API KeybindingService.resolveKeybinding test was failing on macOS with ``` AssertionError: expected { label: '⌃⇧⌥⌘K', …(6) } to deeply equal { label: '⌃⇧⌥⌘K', …(7) } + expected - actual { "WYSIWYG": true "ariaLabel": "⌃⇧⌥⌘K" + "chord": false "dispatchParts": [ "ctrl+shift+alt+meta+K" ] "electronAccelerator": "Ctrl+Shift+Alt+Cmd+K" ``` The `chord: false` was removed from the expected object for other platforms as part of #13217. This fix removes it from the expected object for macOS.
What it does
Upgrades our Monaco dependency to 1.83.1
Fixes #12679
Various changes were necessary, in particular:
setContext
command has been renamed to '_setContext`.vscode-uri
anduri
typings microsoft/vscode#190584StandaloneServices
has not been working for a while, but VS Code instantiating services eagerly instead of lazily has revealed the problemHow to test
A lot of parts of Theia may be affected by this change and need testing, in particular:
I'm opening this PR now in order to facilitate code review, but we still need
Follow-ups
languages-main.ts
Review checklist
Reminder for reviewers