Skip to content

Update to monaco-vscode-api v13 #836

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

Merged
merged 15 commits into from
Jan 28, 2025
Merged

Update to monaco-vscode-api v13 #836

merged 15 commits into from
Jan 28, 2025

Conversation

kaisalmen
Copy link
Collaborator

@kaisalmen kaisalmen commented Jan 25, 2025

Applied changes according to: CodinGame/monaco-vscode-api#561 (comment)

@kaisalmen kaisalmen requested a review from CGNonofr as a code owner January 25, 2025 13:40
@kaisalmen kaisalmen marked this pull request as draft January 25, 2025 13:40
@kaisalmen
Copy link
Collaborator Author

@CGNonofr I see this error when the view service is used in the app playground example in the browser console:
image

and in the "rerender" react component test, but nowhere else in the tests:
image

I don't see this in your deployed demo or anywhere else in the examples here or apart from this one test (I pin-pointed this locally). Any idea why this comes from suddenly? I set dangerouslyIgnoreUnhandledErrors in the vitest conf, so the tests are not reported as failed. They all pass. Just this error is newly reported,

@kaisalmen
Copy link
Collaborator Author

@CGNonofr there are some problems with peerDependencies. I think. I will open an issue https://github.com/CodinGame/monaco-vscode-api and hopefully are able to describe it in an understandable way

@CGNonofr
Copy link
Collaborator

I see this error when the view service is used in the app playground example in the browser console:

I'm tired of that error, it almost as old as monaco itself: microsoft/monaco-editor#4311 but it has no other consequences than an error in the logs 🤷

I don't see this in your deployed demo or anywhere else in the examples here or apart from this one test (I pin-pointed this locally). Any idea why this comes from suddenly? I set dangerouslyIgnoreUnhandledErrors in the vitest conf, so the tests are not reported as failed. They all pass. Just this error is newly reported,

Are you sure? I can see it in https://monaco-vscode-api.netlify.app/, and also locally (just not in full-workbench mode)

there are some problems with peerDependencies.

I'm scared! any insight yet?

@kaisalmen
Copy link
Collaborator Author

I'm scared! any insight yet?

@CGNonofr am still digging. Getting there. Btw, pnpm is very helpful here, because it is way faster (due to its cache) and better standard reporting

@kaisalmen
Copy link
Collaborator Author

Are you sure? I can see it in https://monaco-vscode-api.netlify.app/, and also locally (just not in full-workbench mode)

Maybe my browser cache was not cleaned

@kaisalmen
Copy link
Collaborator Author

@CGNonofr I just pushed an update that builds three very simple projects with npm, pnpm and yarn just to check if basic dependencies work.

@kaisalmen kaisalmen force-pushed the mva-13 branch 2 times, most recently from aeb4bf5 to e302952 Compare January 27, 2025 10:28
@kaisalmen kaisalmen marked this pull request as ready for review January 27, 2025 15:25
@kaisalmen
Copy link
Collaborator Author

@CGNonofr back to the simple dependency approach.

I was thinking about providing a node based script we could export with monaco-languageclient that people could call/use to detect mismatching packages (e.g. nothing else but a specific version likt 13.0.0 should be install from @codingame/monaco-vscode-*). This is likely there more achievable goal and under our control.

@kaisalmen
Copy link
Collaborator Author

@CGNonofr this is now ready for review. The proposed script may come later.

@CGNonofr
Copy link
Collaborator

@CGNonofr back to the simple dependency approach.

I was thinking about providing a node based script we could export with monaco-languageclient that people could call/use to detect mismatching packages (e.g. nothing else but a specific version likt 13.0.0 should be install from @codingame/monaco-vscode-*). This is likely there more achievable goal and under our control.

btw, something can be done at runtime: we store something globally from monaco-vscode-api, so if there is already something in there at load time, throw an error

@kaisalmen
Copy link
Collaborator Author

btw, something can be done at runtime: we store something globally from monaco-vscode-api, so if there is already something in there at load time, throw an error

That is also a good idea

@kaisalmen
Copy link
Collaborator Author

Are you sure? I can see it in https://monaco-vscode-api.netlify.app/, and also locally (just not in full-workbench mode)

I see this as well there. Just tried again with fresh browser and cleared cache.

@cdietrich
Copy link
Contributor

@kaisalmen i am trying to try this out in our yarn based project.
am getting

Error: While cloning /Users/dietrich/myproject/node_modules/monaco-editor-wrapper/node_modules/@codingame/monaco-vscode-9e888134-1a6f-58d9-b0e6-0fc047448366-common -> /Users/dietrich/myproject/node_modules/vscode/node_modules/@codingame/monaco-vscode-api/node_modules/@codingame/monaco-vscode-extensions-service-override/node_modules/@codingame/monaco-vscode-fab30422-b487-5f4e-8d30-8b4d266e3fcd-common/node_modules/@codingame/monaco-vscode-4a36e358-d94d-55e0-86ee-3bcd543d9d3f-common/node_modules/@codingame/monaco-vscode-c465110a-57c0-59d7-a6b2-be0a4db7e517-common/node_modules/@codingame/monaco-vscode-fab30422-b487-5f4e-8d30-8b4d266e3fcd-common/node_modules/@codingame/monaco-vscode-5945a5e2-a66c-5a82-bd2c-1965724b29eb-common/node_modules/@codingame/monaco-vscode-3cf6a388-482f-5484-a806-0525ad9ad8af-common/node_modules/@codingame/monaco-vscode-f48982c4-9e82-55e2-b800-20e6d1e6096f-common/node_modules/@codingame/monaco-vscode-5108c2c9-4ada-52d8-8c4b-fe03b3160e71-common/node_modules/@codingame/monaco-vscode-219d9a5f-b446-507b-a188-1178a0867c75-common/node_modules/@codingame/monaco-vscode-9e888134-1a6f-58d9-b0e6-0fc047448366-common ENAMETOOLONG: name too long, copyfile '/Users/dietrich/myproject/node_modules/monaco-editor-wrapper/node_modules/@codingame/monaco-vscode-9e888134-1a6f-58d9-b0e6-0fc047448366-common/vscode/src/vs/workbench/browser/parts/editor/editorPane.d.ts' -> '/Users/dietrich/myproject/node_modules/vscode/node_modules/@codingame/monaco-vscode-api/node_modules/@codingame/monaco-vscode-extensions-service-override/node_modules/@codingame/monaco-vscode-fab30422-b487-5f4e-8d30-8b4d266e3fcd-common/node_modules/@codingame/monaco-vscode-4a36e358-d94d-55e0-86ee-3bcd543d9d3f-common/node_modules/@codingame/monaco-vscode-c465110a-57c0-59d7-a6b2-be0a4db7e517-common/node_modules/@codingame/monaco-vscode-fab30422-b487-5f4e-8d30-8b4d266e3fcd-common/node_modules/@codingame/monaco-vscode-5945a5e2-a66c-5a82-bd2c-1965724b29eb-common/node_modules/@codingame/monaco-vscode-3cf6a388-482f-5484-a806-0525ad9ad8af-common/node_modules/@codingame/monaco-vscode-f48982c4-9e82-55e2-b800-20e6d1e6096f-common/node_modules/@codingame/monaco-vscode-5108c2c9-4ada-52d8-8c4b-fe03b3160e71-common/node_modules/@codingame/monaco-vscode-219d9a5f-b446-507b-a188-1178a0867c75-common/node_modules/@codingame/monaco-vscode-9e888134-1a6f-58d9-b0e6-0fc047448366-common/vscode/src/vs/workbench/browser/parts/editor/editorPane.d.ts'

@cdietrich
Copy link
Contributor

hmmmm the next4 actually solves it ;)

@kaisalmen
Copy link
Collaborator Author

@CGNonofr npm dependency resolution with 13.1.0 is a lot quicker again.

@cdietrich
Copy link
Contributor

hmmm
the

"vscode": "npm:@codingame/[email protected]",

will destorys our multi project setup.
we have other modules that should take the real vscode

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Jan 28, 2025

@cdietrich vscode is currently defined as regular dependency in monaco-languageclient, see the main:
https://github.com/TypeFox/monaco-languageclient/blob/main/packages/client/package.json#L73

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Jan 28, 2025

@cdietrich you have to adjust your code, see #836 (comment)

Maybe you should wait a bit and use a stable build. The CHANGELOGs are also not yet complete.

@cdietrich
Copy link
Contributor

cdietrich commented Jan 28, 2025

so you assume it should work once you release?

it complains about the code that really uses vscode, not in the modules that use monco-editor-wrapper

@kaisalmen
Copy link
Collaborator Author

it complains about the code that really uses vscode, not in the modules that use monco-editor-wrapper

@cdietrich have you applied the changes mentioned here: CodinGame/monaco-vscode-api#561 (comment)

You early feedback is of course appreciated. I haven't written that down yet ⬆️ and I have to make up my mind if we have to perform a major version upgrade, but the api on this level is not changed, so a x.2.0 release should be fine.

@cdietrich
Copy link
Contributor

I am still confused what of that applies to the module that uses Monaco and what to the others

@kaisalmen
Copy link
Collaborator Author

I am still confused what of that applies to the module that uses Monaco and what to the others

Whatever you do with the api of monaco-editor itself is not effected. Have you looked at the delta in our examples. The changes are pretty straight-forward and the way the dependencies are used has not really changed. Is your code openely available?

@cdietrich
Copy link
Contributor

cdietrich commented Jan 28, 2025

no code is not available.
and i fear i wont find time to create a duplicate that can be shared soonish

we have a structure like this

  • langium dsl
  • web thingy that uses monaco
  • vscode extension
  • util used in vscode extension

in your pr i e.g dont see any changes to https://github.com/TypeFox/monaco-languageclient/tree/main/packages/examples
at all (but i also assume nothing of that uses langium + vscode

when i now would update imports on the vscode extension
and we bundle it with vscode:provided, wont that break

@samir-abis maybe you can explain better what we do

maybe it works with the current stuff just accidentially

@kaisalmen
Copy link
Collaborator Author

in your pr i e.g dont see any changes to https://github.com/TypeFox/monaco-languageclient/tree/main/packages/examples
at all (but i also assume nothing of that uses langium + vscode

@cdietrich There are 86 files changed in this PR and there are many changes in the examples. What are you looking at?

@cdietrich
Copy link
Contributor

sry if i was imprecise. i just see changes on stuff that uses monaco.

we e.g. have

packages/our-language-vscode/src/notebook/our-notebook-kernel.ts

with

import * as vscode from "vscode"
class CustomNotebookCellStatusBarItem implements vscode.NotebookCellStatusBarItem {

am i supposed to import from some codingame here?

@cdietrich
Copy link
Contributor

cdietrich commented Jan 28, 2025

on our main node_modules folder

./@codingame/monaco-vscode-api/api.js
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHostTypes.js
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHostTypeConverters.js
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHost.api.impl.js
./@types/vscode/index.d.ts
./vscode/api.js
./vscode/vscode/src/vs/workbench/api/common/extHostTypes.js
./vscode/vscode/src/vs/workbench/api/common/extHostTypeConverters.js
./vscode/vscode/src/vs/workbench/api/common/extHost.api.impl.js

on the next version one

./@codingame/monaco-vscode-extension-api/extension.api.js
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHostTypes.js
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHostTypeConverters.js
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHostTypes.d.ts
./@codingame/monaco-vscode-api/vscode/src/vs/workbench/api/common/extHost.api.impl.js
./@codingame/monaco-vscode-api/vscode-dts/vscode.d.ts
./@types/vscode/index.d.ts
./monaco-editor-wrapper/node_modules/vscode/extension.api.js
./monaco-languageclient/node_modules/vscode/extension.api.js
./vscode/vscode/src/vs/workbench/api/common/extHostTypes.js
./vscode/vscode/src/vs/workbench/api/common/extHostTypeConverters.js
./vscode/vscode/src/vs/workbench/api/common/extHostTypes.d.ts
./vscode/vscode/src/vs/workbench/api/common/extHost.api.impl.js
./vscode/vscode-dts/vscode.d.ts

@kaisalmen
Copy link
Collaborator Author

am i supposed to import from some codingame here?

@cdietrich You always did in the past if you used monaco-editor-wrapper. monaco-languageclient had the remapped vscode package "vscode": "npm:@codingame/[email protected]" as dependency and made it thereby available to the inheriting packages. Now the @types/vscode is no longer needed, because the type definitions are now directly included in the @codingame/monac-vscode packages. This changed with 13.1.x.

@CGNonofr this PR is now ready for merge. -next.5 builds are available

@cdietrich
Copy link
Contributor

cdietrich commented Jan 28, 2025

i guess i need help on this from someone else in my team.
i still have no clue which imports in which packages i need to change to which.

@cdietrich
Copy link
Contributor

cdietrich commented Jan 28, 2025

hmmm checked out your branch and i see this
Bildschirmfoto 2025-01-28 um 15 33 16
Bildschirmfoto 2025-01-28 um 15 33 44

so is this one correct:
"vscode": "npm:@codingame/[email protected]"

i nguess i need to figure out where the bad one is coming from

head node_modules/vscode/package.json 
{
  "name": "@codingame/monaco-vscode-extension-api",
  "version": "13.1.1",

so it picks the extension-api one for us

@kaisalmen
Copy link
Collaborator Author

What does npm list vscode tell you or a similar command from the respective packager manager you use

@cdietrich
Copy link
Contributor

in yarn berry it wont find anything. am not sure if there is a command line option to show it

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Jan 28, 2025

@cdietrich IMO do yourself a favour and get rid of yarn. Use pnpm if you want a cache or just npm

@CGNonofr CGNonofr self-requested a review January 28, 2025 16:06
@kaisalmen kaisalmen merged commit 6b7c10f into main Jan 28, 2025
2 checks passed
@kaisalmen kaisalmen deleted the mva-13 branch January 28, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants