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

Update Monaco #10387

Closed
tsmaeder opened this issue Nov 5, 2021 · 8 comments · Fixed by #10736
Closed

Update Monaco #10387

tsmaeder opened this issue Nov 5, 2021 · 8 comments · Fixed by #10736
Assignees
Labels
monaco issues related to monaco vscode issues related to VSCode compatibility

Comments

@tsmaeder
Copy link
Contributor

tsmaeder commented Nov 5, 2021

Feature Description:

We should update the monaco editor once again.

@tsmaeder tsmaeder added monaco issues related to monaco vscode issues related to VSCode compatibility labels Nov 5, 2021
@tsmaeder
Copy link
Contributor Author

We should really align the versioning of the vs code we use (http://github.com/theia-ide/vscode) with the VS code tags. Right now it's not obvious which version we use.

@colin-grant-work colin-grant-work self-assigned this Feb 1, 2022
@colin-grant-work
Copy link
Contributor

colin-grant-work commented Feb 1, 2022

Issues and Desiderata

This is a list of things that are troublesome about the way we consume Monaco using our current code - the building / importing / running part, rather than features. Feel free to add your own comments, and I'll add and edit as I try to work through these issue.

Relationship to Standalone

Problem

From VSCode's perspective, the monaco-editor-core package based on the standalone area of VSCode's repo is the release version of Monaco. Rather than restricting ourselves to what's available from the standalone release, we bring in code from other areas of VSCode, and release our own @theia/monaco-editor-core package. That makes it difficult to know what exactly we're using, because it doesn't match anybody's package definition but our own. This also makes it hard to understand what our version numbers mean, if anything.

Desiderata

  • Minimum: better documentation of the @theia/monaco-editor-core package. The NPM page links to this repository, for example, which is not where that code lives at all. There's scattered information about how we build it, but it's not easy to find and is quite out of date.
  • From any commit in Theia, it should be easy to determine exactly what Monaco code is running in Theia.
  • It should be easy to experiment with different versions / builds of Monaco from inside Theia.

Transparency of Consumption

Problem

VSCode is built to expect that it will be loaded using an Asynchronous Module Loader. As a consequence, we can't include it in our webpack build, and we can't use normal import statements to include the Monaco code in our code, and so we load the Monaco code separately, and we gain access to it using a monaco field on the global object. For typings, we write declarations by hand into a declaration file as the need arises to refer to different elements already present in the Monaco code we consume.

Desiderata

  • Typing not maintained by hand
  • Types are comprehensive: we know everything we have access to without having to look at the VSCode repo.
  • Nice to have: Monaco imported the same as any other package, packaged by webpack.
  • Nicer to have: Monaco imported the same as any other package, Theia more agnostic about packaging and resolution.

Notes

I know that the monaco-editor build includes ESM modules that can be loaded with webpack. I believe that recent monaco-editor-core builds do as well. If so, that would be one item off the list immediately :-).

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Feb 2, 2022

  1. Kudos for weaving the work "desiderata" into this ;-)
  2. I don't believe we should be maintaining the monaco.d.ts file by hand: theia-ide/vscode@e930e42#diff-25a6634263c1b1f6fc4697a04e2b9904ea4b042a89af59dc93ec1f5d44848a26

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Feb 2, 2022

Strategically, I believe it was a mistake to ever rely on anything outside the standard packaged monaco build: the time we're saving we're spending each time we update monaco.

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Feb 10, 2022

@tsmaeder, I agree. We have a decision to make with this uplift, now. monaco-editor-core does now include a set of ES module files, so we can consume those. It also has an official API for how the package should be used. We are digging into a lot of stuff that is not part of that public API, and it's fairly trivial to make the VSCode build output declaration files for the private API's in addition to the public API's. Reconciling our code even just the with the actual private API's will take a while - I estimate another couple of days just to get TS to build it, and an indeterminate while longer to actually get it to work. We could also refactor not to use any private API's and go exclusively with the public face. That would definitely save us work down the road - we'd be insulated against changes in the private area - but I don't know for certain how much work it would take or whether we'd actually lose any functionality that we absolutely need. I'm curious what you (and others, @paul-marechal , @vince-fugnitto, @JonasHelming, @msujew) think about those approaches. I'll post my work-in-progress (not to say, by any stretch of the imagination, 'working' :-) ) branch so people can follow the progress and consider the alternatives with some context.

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Feb 10, 2022

I've made a PR that isn't worth looking at and won't be for a while, but I have been recording some thoughts and notes in packages/monaco/uplift-journal.md, and there are some details there about the effects of the public-private API distinction on the work. There are also instructions to replicate the dev set up (at the moment I'm just pointing to a local build in the packages/monaco/package.json) if anyone wanted to replicate that.

@tsmaeder
Copy link
Contributor Author

I've made a PR that isn't worth looking

I'll keep my itchy feet still, then ;-).

About going with only the public monaco API: I was thinking if we might be better off just copying the bits we're using from VS code that are private? I think it might make it a it harder to profit from further improvements in those areas, but on the other hand, updating monaco would become much easier. @colin-grant-work do you have a handle on how much code we're actually using that's off-API?

@colin-grant-work
Copy link
Contributor

@colin-grant-work do you have a handle on how much code we're actually using that's off-API?

@tsmaeder, there are two or three questions here:

  1. How often are we referring to non-API code / interfaces?
  2. How often are we using non-API code?
  3. How often do we need to be using non-API code to get done what we need to get done?

At the moment, I can only answer the first roughly, and the answer is 'fairly often.' I would say that the majority of files in which we refer to Monaco require references to non-API objects. But as I go through and try to reconcile our code with the newly-known types, I'm able to reduce that frequency somewhat by pushing the references to non-API items up the chain and then recasting as the public API object. That means the answer to the second question is likely 'less often than it currently appears,' and a careful look at the VSCode code and judicious restructuring of ours may mean that we're able to cut that down even further. I'll know better where we stand on the second question, at least, in a day or two.

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

Successfully merging a pull request may close this issue.

2 participants