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

Upgrade @theia/monaco-editor-core (currently 0.20) to the latest Monaco (currently standalone/0.23.x) #8969

Closed
danarad05 opened this issue Jan 19, 2021 · 71 comments
Assignees
Labels
monaco issues related to monaco

Comments

@danarad05
Copy link
Contributor

danarad05 commented Jan 19, 2021

Currently these feature are missing from Theia monaco-editor-core (obviously could be other features as well):

  1. "in" operator for when clauses was added in v1.49 (ticket Support new vscode 'in' operator for 'when' clauses #8691 needs it)
  2. vscode URI utils functions like joinPath (ticket vscode.Uri.joinPath is not a function #8752 needs it)

References:
Migration basic Guidelines - https://github.com/eclipse-theia/theia/wiki/LSP-and-Monaco-integration#migration-guidelines
#5412 (comment) - althought I think that is outdated as current repo is @theia/monaco-editor-core and not TypeFox/monaco-editor-core.

@amiramw
Copy link
Member

amiramw commented Jan 19, 2021

Previous upgrade for reference: #8010

@azatsarynnyy
Copy link
Member

Thank you @danarad05 for registering the issue.
I believe @RomanNikitenko can share his knowledge and experience he has got during a couple of the latest Monaco upgrades.
It would be useful for anyone who will decide to do the subsequent upgrades.
It would be great to have all the required steps described somewhere on the wiki, maybe update the existing Migration Guidelines.

@vince-fugnitto vince-fugnitto added the monaco issues related to monaco label Jan 19, 2021
@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jan 20, 2021

The main required steps for Monaco upgrade:

  • publish a new version of @theia/monaco-editor-core and apply this dependency for theia, please see details here: upgrade to Monaco 0.20.0 #7697 (comment)
  • the upgrade process has been greatly simplified by removing monaco-languageclient dependency, so we don't need upgrade monaco-languageclient like here this time, but maybe some dependencies related to Monaco should be updated like: vscode-ws-jsonrpc
  • signatures alignment (signatures exist and matching in a new version of Monaco) - I provided references to the corresponding types in VS Code at the previous Monaco upgrade - it should simplify the process of searching the corresponding types
  • editor configurations alignment, please see instructions
  • testing/fixing. Please see How to test section for the previous PR Upgrade to Monaco 0.20.0 #8010. Also some time ago @akosyakov provided integration tests: add integration tests for typescript language #7265. It helps to test the changes

@azatsarynnyy

It would be great to have all the required steps described somewhere on the wiki, maybe update the existing Migration Guidelines.

I described the required steps above, I'm going to add some items within the next upgrade if I missed something. Also it will be the first Monaco upgrade after removing monaco-languageclient dependency.
I think within coming Monaco upgrade we could go through the provided steps ^ first and then update the corresponding wiki page.
WDYT?

@azatsarynnyy
Copy link
Member

I think within coming Monaco upgrade we could go through the provided steps ^ first and then update the corresponding wiki page.
WDYT?

Agree 👍

@amiramw
Copy link
Member

amiramw commented Feb 14, 2021

Assigned this to @danarad05. If someone else started looking into it, please let us know.

@danarad05
Copy link
Contributor Author

@RomanNikitenko
Hi Roman,

  1. I'm not sure I understand the first step here - currently there isn't a standalone/0.22.x branch in theia-ide/vscode.
    Could you elaborate what are prior steps to this first step?
  2. Also, what version is relevant for standalone/0.22.x? https://github.com/microsoft/vscode/releases/tag/1.53.2?

image

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Feb 15, 2021

@danarad05

Hello!
https://github.com/theia-ide/vscode was forked from https://github.com/microsoft/vscode
so you should fetch the corresponding branch to https://github.com/theia-ide/vscode from the fork first.

vscode_branches

@danarad05
Copy link
Contributor Author

@RomanNikitenko
fetched from upstream = microsoft/vscode, updated the disable treeshake and monaco package.json but couldn't push changes - don't have required permissions.
Please advise,
Thanks

image

@RomanNikitenko
Copy link
Contributor

@danarad05
I checked and faced with the same permission error

permissions_error

@akosyakov please take a look #8969 (comment)

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Feb 15, 2021

@kittaakos @marcdumais-work @vince-fugnitto

Maybe someone knows who has the required permissions to publish a new version of @theia/monaco-editor-core.

The problem is: at the current step @danarad05 can not push the changes to https://github.com/theia-ide/vscode.
The next step - publishing a new version of @theia/monaco-editor-core to the npm registry.
Please see the comments above.

Thanks in advance!

@marcdumais-work
Copy link
Contributor

@RomanNikitenko I have just given @danarad05 write access to that repo - just need to accept GH's invite,

@marcdumais-work
Copy link
Contributor

(and you too @RomanNikitenko, in case you need it)

@akosyakov
Copy link
Member

@RomanNikitenko You will need to fine someone else to review. I don't have time for Theia recently.

@danarad05
Copy link
Contributor Author

danarad05 commented Feb 16, 2021

but maybe some dependencies related to Monaco should be updated like: vscode-ws-jsonrpc

@RomanNikitenko
I don't see any usage of vscode-ws-jsonrpc on standalone/0.22.x. Do you mean to add it to 0.22.x use it instead of exisiting WebSocket?

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Feb 16, 2021

@danarad05
About

the upgrade process has been greatly simplified by removing monaco-languageclient dependency, so we don't need upgrade monaco-languageclient like here this time, but maybe some dependencies related to Monaco should be updated like: vscode-ws-jsonrpc

I meant that probably we have to update some dependencies on https://github.com/eclipse-theia/theia side.

I think we don't need any other changes for standalone/0.22.x of https://github.com/theia-ide/vscode except theia-ide/vscode@863cb53.

The next step is publishing a new version of @theia/monaco-editor-core to the npm registry and applying it to theia.

@danarad05
Copy link
Contributor Author

@RomanNikitenko

Not able to publish to npmjs. Please advise.

npm notice === Tarball Details ===
npm notice name: @theia/monaco-editor-core
npm notice version: 0.22.3
npm notice package size: 8.1 MB
npm notice unpacked size: 43.1 MB
npm notice shasum: 2de06f18b8dd3461786b53ab9a7a6282808063bb
npm notice integrity: sha512-VmylSNp2/F0F3[...]8nssT0snEPZQw==
npm notice total files: 663
npm notice
npm ERR! code E404
npm ERR! 404 Not Found - PUT https://registry.npmjs.org/@theia%2fmonaco-editor-core - Not found
npm ERR! 404
npm ERR! 404 '@theia/monaco-editor-core@0.22.3' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.
npm ERR! A complete log of this run can be found in:
npm ERR! C:\Users\C5313341\AppData\Roaming\npm-cache_logs\2021-02-18T09_21_31_859Z-debug.log

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Feb 18, 2021

@danarad05
Copy link
Contributor Author

danarad05 commented Feb 18, 2021

@RomanNikitenko
@amiramw
I logged into npm successfully before npm publish and got this error. I tried now various things but still didn't manage to publish successfully. Are the permissions needed to be able to publish to that npm package?

@marcdumais-work
Copy link
Contributor

Hi @danarad05 ,

Are the permissions needed to be able to publish to that npm package?

I think so. Did someone provide you with an npm publish token for @Theia? If not you probably need one.

@RomanNikitenko
Copy link
Contributor

I don't have an npm publish token for @Theia - at the previous Monaco upgrade Anton published @theia/monaco-editor-core to the npm registry.

AFAIK @danarad05 prepared the corresponding branch for the publishing.
So, someone, who has the token, could help @danarad05 with it - publish it using the branch or provide the token.

@marcdumais-work
Copy link
Contributor

One option would be to add the publish token as a secure variable or whatever it's called and publish from a GitHub action. WDYT?

@marcdumais-work
Copy link
Contributor

BTW, would it make sense for that repo to be hosted at the Foundation? Similarly to https://github.com/eclipse-theia/vscode-builtin-extensions?

@RomanNikitenko
Copy link
Contributor

One option would be to add the publish token as a secure variable or whatever it's called and publish from a GitHub action. WDYT?

Sounds good for me!

@benoitf @azatsarynnyy
please share your opinion about:

@azatsarynnyy
Copy link
Member

One option would be to add the publish token as a secure variable or whatever it's called and publish from a GitHub action. WDYT?

you mean a GitHub secret? sounds good 👍

@marcdumais-work
Copy link
Contributor

@eclipse-theia/ecd-theia-committers can anyone can help Dan?

@RomanNikitenko
Copy link
Contributor

Dan, unfortunately I have no experience in the mentioned above problem - so - sorry - I can not provide any help/advise.

@azatsarynnyy
Copy link
Member

If I understood correctly from today's Theia Dev meeting, @JonasHelming's team is going to look at the ES6 migration issue.

@danarad05
Copy link
Contributor Author

danarad05 commented Mar 30, 2021

Thank you @marcdumais-work @RomanNikitenko @azatsarynnyy. Unfortunately I couldn't attend this week's meeting.

Also, I might have managed to compile standalone/0.23.x to ES5 successfully - with doing some changes to code (trying to do minimal changes) - still checking (- and still it doesn't mean that is way to go - but wanted to see if possible).

For instance - compile completed successfully although TS file shows TS problem:
image

Not sure yet if Object.values() is actually working - will try to test it tomorrow:
image

@danarad05
Copy link
Contributor Author

@marcdumais-work @RomanNikitenko @azatsarynnyy @offer8

Please check changes I've made to @theia/monaco-editor-core for compiling to ES5: theia-ide/vscode@07fb6e1

Seems to me as changes we could live with (?) - if all is working properly. If there are objections for this route - please let me know now.
I have also published it to npmjs.org as 0.23.0-next-82e8ea39fc101d63.2 version - in order to be able to consume it in Theia.

I will now test it in Theia and let you know

@paul-marechal
Copy link
Member

@danarad05 rather than modifying the TS sources should you transpile the JS output from ES6 to ES5?

This is what @akosyakov had done if I understand correctly:

{
test: /\\.js$/,
// include only es6 dependencies to transpile them to es5 classes
include: /vscode-ws-jsonrpc|vscode-jsonrpc|vscode-languageserver-protocol|vscode-languageserver-types/,
use: {
loader: 'babel-loader',
options: {
presets: ['@babel/preset-env'],
plugins: [
// reuse runtime babel lib instead of generating it in each js file
'@babel/plugin-transform-runtime',
// ensure that classes are transpiled
'@babel/plugin-transform-classes'
],
// see https://github.com/babel/babel/issues/8900#issuecomment-431240426
sourceType: 'unambiguous',
cacheDirectory: true
}
}
}

@danarad05
Copy link
Contributor Author

@marechal-p
The code you quoted is from Theia. I'm talking about @theia/monaco-editor-core there there isn't usage of babel

@paul-marechal
Copy link
Member

paul-marechal commented Apr 1, 2021

I quoted a Theia configuration that is used to consume ES6. If Theia can consume ES6 that way maybe you don't need to change monaco's target from ES6 to ES5 then. Since we already transpile in Theia?

@danarad05
Copy link
Contributor Author

Since we already transpile in Theia

  1. We don't transpile node_modules and so the monaco.* interfaces that we defined in Theia are referencing @theia/monaco-editor-core library that is transpiled to ES6 - which causes initialization issues like this:
    image.

  2. I tried before doing these changes (theia-ide/vscode@07fb6e1) to add babel and was unsuccessful with it - it still gave me errors and wouldn't complete compilation successfully. Don't know if I did it wrong and if there is a way to do it that successfully..?

In any case - even if we can refrain for the changes I've done, we still need to transpile to ES5 the library as it won't be transpiled by Theia (as long as Theia is configured for ES5).

@caseyflynn-google
Copy link
Contributor

Hey All!

Is there any update on this? We are hoping this will resolve #8752 so we may update the Go VS Code extension in our deployment. Is there anything we can do to assist?

@danarad05
Copy link
Contributor Author

Hi @caseyflynn-google

I hope that in a few days/week we'll have a complete solution which consists of 2 parts:

  1. First part (this issue) @theia/monaco-editor-core upgraded to 0.23.x - which is already ready.
  2. Second part 8969-upgrade-monaco-editor-core-to-standalone-0.23.x #9154 - which are changes required in Theia itself - which includes replacing quickOpen modules with quickInput modules - as 0.23.x does not support quickOpen anymore.

When #9154 will be completed we would need a lot of testing as this is a large refactoring. So your help with that would be appreciated.

Thanks

@azatsarynnyy
Copy link
Member

@danarad05 from the Red Hat side, we have a couple of people to help with testing and reviewing the contribution.
Just to let you know in case help is needed.

@danarad05
Copy link
Contributor Author

@caseyflynn-google @westbury
cc: @marcdumais-work

Is there any update on this? We are hoping this will resolve #8752 so we may update the Go VS Code extension in our deployment. Is there anything we can do to assist?

PR #9154 is ready for review / testing - if you would like to join in the effort - you are welcome.

Thanks

@tsmaeder
Copy link
Contributor

tsmaeder commented Jul 2, 2021

Reopening....I have a follow-up refactoring we decided to do post-merge of the first PR.

@tsmaeder
Copy link
Contributor

Followup refactoring now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco
Projects
None yet
Development

No branches or pull requests