Update to vscode v1.82.0 monaco-editor v0.43.0

[kaisalmen]

Following the guide:

What I usually do is:

• Get the new vscode commit from the new monaco-editor release version (in the package.json there is a vscodeRef field)
• Open the vscode repo, reset to the previous vscodeRef commit
• Apply the current patch: patch -p1 < /path/to/monaco-vscode-api/scripts/vscode.patch
• git stash
• checkout new vscodeRef commit
• git stash pop
• resolve conflicts, update code...
• generate new patch: git diff --staged > /path/to/monaco-vscode-api/scripts/vscode.patch

Now on the monaco-vscode-api side:

• update monaco-editor (and other dependencies) and update to the new vscodeRef
• wait for the new vscode version to be downloaded and built
• Fix errors, adapt code, build, include the vscode.patch into this commit
• update demo

[kaisalmen]

@CGNonofr Finished the vscode patch part

[kaisalmen]

@CGNonofr I will adapt the guide above while I proceed.

[kaisalmen]

@CGNonofr I am not sure the patch creation was good. I resolved conflicts in those files:

before creating the diff

[CGNonofr]

@CGNonofr I am not sure the patch creation was good. I resolved conflicts in those files: before creating the diff

What do you mean by the patch creation was good? What did you do?

[kaisalmen]

@CGNonofr After updating the dependencies and having the patch in place I see 1326 compile errors locally. Theses are one or two orders of magnitude more errors than I expected. 😅

[CGNonofr]

@kaisalmen Still no clear to me what you mean :x What does that mean to the patch in place? also the patch seems to be empty in this PR

[kaisalmen]

What does that mean to the patch in place?

./scripts/vscode.patch was replaced. git diff > /path/to/monaco-vscode-api/scripts/vscode.patch overrides whatever was there, right?

also the patch seems to be empty in this PR

No, why do you think that?

Can you check out the branch and reproduce? Or shall I create the patch once more and see if the results are the same?

[CGNonofr]

I don't exactly know what you did :D here's the patch on this PR: https://github.com/CodinGame/monaco-vscode-api/blob/403d3d0b1910fa5e2168a4888ed50b4e2451e4d1/scripts/vscode.patch (empty)

[kaisalmen]

It is not empty here: https://github.com/CodinGame/monaco-vscode-api/pull/189/files#diff-2533526504623ad6913cf6b8c9521bf217b521fbef7426a43a4944d3da48261b
I don't understand

[kaisalmen]

Damn it, sorry. It is

[kaisalmen]

--staged was missing from the diff command

[kaisalmen]

@CGNonofr Another thing I might have done wrong: I used the vscode.patch from the 1.81.7 tag, but actually it must be the one from 1.81.0, correct?

[CGNonofr]

@CGNonofr Another thing I might have done wrong: I used the vscode.patch from the 1.81.7 tag, but actually it must be the one from 1.81.0, correct?

You should take the patch from main, why using an old one?

[kaisalmen]

You should take the patch from main, why using an old one?

Where did the intermediate vscode.patch updates result from? The process is not clear to me.

Btw, there is an error in the patch. I am just rebuilding locally.

[CGNonofr]

You should take the patch from main, why using an old one?

Where did the intermediate vscode.patch updates result from? The process is not clear to me.

Btw, there is an error in the patch. I am just rebuilding locally.

What do you mean by intermediate?

There is multiple way to achieve it though:

• You can apply the old patch on the new vscode version, it will tell you in the console what it failed to apply because it has changed since then
• You can apply the patch on the version it was created on (so no error), then switch to the new version using git, either:
  • By stashing the changes, checkout the new tag then unstashing the changes
  • By creating a fake commit including every changes, then rebasing on the new vscode tag, then resetting just before your fake commit

The first way seems dangerous to me though as it's easy to lose changes though, git is way better at it

[kaisalmen]

What do you mean by intermediate?

You updated the patch intermediately after 1.81.0 release of this lib. I was just wondering why or what is the root-course?

[kaisalmen]

@CGNonofr I solved almost half of the compilation problems. Understanding some of the changes without context just by looking at git diffs is hard.

[CGNonofr]

@CGNonofr I solved almost half of the compilation problems. Understanding some of the changes without context just by looking at git diffs is hard.

Agreed! having a git history explaining the changes would be nice, but I don't exactly know how to have that

[kaisalmen]

I use Fork to compare directly the tags and see what has changed, but even this way there are hundreds of files. Other visual tools can do the same.

[kaisalmen]

@CGNonofr there are a few compile errors left. Maybe you have an idea? Also, you should check if my changes to missing-services.ts are really good.

It will be helpful to have some breadcrumbs / idea-box how to best analyse / tackle the changes in between two vscode tags. WDYT?

[CGNonofr]

@CGNonofr there are a few compile errors left. Maybe you have an idea? Also, you should check if my changes to missing-services.ts are really good.

They are! Do not hesitate to run the eslint autofix, it gets rid of the majority of your errors

I've just realized we need to run npx @vscode/dts dev after an update (to upate the vscode.proposed.xxx.d.ts files): it fixes the error in api.ts

The remaining errors in missing-services.ts are not hard to fix

It remains the errors in quickcess.ts which are just a import path to update and a new method to add

It then remains a missing dependency and that's it

I've added a commit with the fixes, feel free to rewrite it as you wish

I've not tested it at all yet though

It will be helpful to have some breadcrumbs / idea-box how to best analyse / tackle the changes in between two vscode tags. WDYT?

What do you have in mind?

[kaisalmen]

@CGNonofr thank you!
Do you see those errors too?

What do you have in mind?

Basically expand the "upgrade guide" with hints / example of what to do. I can incorporate some of your more general hints above, for example.

[CGNonofr]

@CGNonofr thank you! Do you see those errors too?

It looks like there is 5 new services to add in missing-services.ts

What do you have in mind?

Basically expand the "upgrade guide" with hints / example of what to do. I can incorporate some of your more general hints above, for example.

Yep great idea!

[kaisalmen]

@CGNonofr sorry there was not further progress. I am busy with other things currently. Maybe I have more time end of the week?

[CGNonofr]

@CGNonofr sorry there was not further progress. I am busy with other things currently. Maybe I have more time end of the week?

No problem, same problem here.

[kaisalmen]

@CGNonofr I have added the missing service and started the upgrade guide (WIP).

Have you seen this already in the demo (don't know if it is new)?

[kaisalmen]

@CGNonofr this is ready for review. Are you happy with a doc directory for the upgrade guide or should it be moved elsewhere?
I have not yet investigated the tsserver error above ⬆️

[CGNonofr]

@CGNonofr this is ready for review. Are you happy with a doc directory for the upgrade guide or should it be moved elsewhere? I have not yet investigated the tsserver error above ⬆️

That path is fine! that's perfect

The documentation itself could be a little more explicit/detailed but that's a good starting point

we need to have that tsserver error fixed though

[kaisalmen]

The documentation itself could be a little more explicit/detailed but that's a good starting point

It will evolve! 🙂

we need to have that tsserver error fixed though

I will look into it later tonight or tomorrow.

[kaisalmen]

If I remove this hack:
https://github.com/CodinGame/monaco-vscode-api/blob/main/demo/vite.config.ts#L30-L32
I end up here

Can this whole error be somehow linked to this change?
microsoft/vscode#191019

Any idea? I currently don't know how to fix this.

[CGNonofr]

If I remove this hack: https://github.com/CodinGame/monaco-vscode-api/blob/main/demo/vite.config.ts#L30-L32 I end up here

Can this whole error be somehow linked to this change? microsoft/vscode#191019

Any idea? I currently don't know how to fix this.

Yeah this hack can now be safely removed, I've already removed it in one of my PRs

They seem to have change things a lot indeed

I don't know what is the exact issue, nor why I'm not able to reproduce in vscode.dev. They added a lot of console.log and they are not there on vscode.dev either. I even tried to use the same commit as the one published on vscode.dev... Maybe they don't use the last typescript-language-feature extension version?

btw, it doesn't seem to have consequences and it's clear that there is a missing case 'action::watchTypingLocations':, we can still hotfix it in the patch 🤷 (and the same for the console.log?)

[CGNonofr]

Ok more details:

• The difference between vscode.dev and here is that this function returns true here vs false in vscode.dev. SharedArrayBuffer are not available in vscode.dev. It's probably related to Enable project-wide JS/TS IntelliSense on web microsoft/vscode#170920
• In vite.config.ts, if you change the cdnDomain to 'http://127.0.0.1:5173' (so the same domain at the main page), the error and the log message disapear
• Another way to suppress the errors and logs: add "typescript.tsserver.web.projectWideIntellisense.enabled": false in the settings

So my conclusion is that the feature is buggy/verbose, nobody tested it because vscode.dev doesn't use it anyway. It will probably be fixed in the future.

I'm not sure either what they mean by projectWideIntellisense. Even off, everything seems to work just fine.

Maybe let's just disable projectWideIntellisense in the demo with a comment explaining the reasons

edit: some explanations here: https://code.visualstudio.com/docs/nodejs/working-with-javascript#_partial-intellisense-mode

Adding the case still looks like a good idea, as otherwise the extension keep loading indefinitely when projectWideIntellisense is enabled

[CGNonofr]

(I've rebased my PRs on yours)

[kaisalmen]

@CGNonofr Thank you for your investigation. 👍 🎉
I have updated the patch and integrated the projectWideIntellisense setting. Actually with the added case in the patch the error is gone even when set to true and it seems to work just fine.

I can no longer push directly to this repo. Can you use this branch and merge this PR (if you are ok with the content)?
https://github.com/kaisalmen/monaco-vscode-api/tree/vscode-1.82.0-monaco-0.43.0

Idea: Whenever this PR is ready, you merge it, but hold new releases until the other PRs are in. I would like to test the whole stack of changes tomorrow with monaco-languageclient. Can you release another next release once everything is rebased once more?

[CGNonofr]

@CGNonofr Thank you for your investigation. 👍 🎉 I have updated the patch and integrated the projectWideIntellisense setting. Actually with the added case in the patch the error is gone even when set to true and it seems to work just fine.

I can no longer push directly to this repo. Can you use this branch and merge this PR (if you are ok with the content)? https://github.com/kaisalmen/monaco-vscode-api/tree/vscode-1.82.0-monaco-0.43.0

Idea: Whenever this PR is ready, you merge it, but hold new releases until the other PRs are in. I would like to test the whole stack of changes tomorrow with monaco-languageclient. Can you release another next release once everything is rebased once more?

note that the default value for projectWideIntellisense.enabled is already true

[kaisalmen]

note that the default value for projectWideIntellisense.enabled is already true

@CGNonofr Should I remove it then? I cannot put a comment directly above as it will break the JSON. Are you ok with the patch and the other changes?

[CGNonofr]

note that the default value for projectWideIntellisense.enabled is already true

@CGNonofr Should I remove it then?

I don't think there is any more reason for it to be there compared to previous version

I cannot put a comment directly above as it will break the JSON.

Does it? it seems it supports comments in json

Are you ok with the patch and the other changes?

Yup ! :)

[kaisalmen]

I don't think there is any more reason for it to be there compared to previous version

Removed with last force push. Pushing directly It works gain. This is ready now ⬆️

Does it? it seems it supports comments in json

so, it is actually jsonc 😉