Skip to content

Update to monaco-vscode-api 1.76 - #470

Merged
kaisalmen merged 9 commits into
mainfrom
update-monaco-vscode-1.76
Apr 4, 2023
Merged

Update to monaco-vscode-api 1.76#470
kaisalmen merged 9 commits into
mainfrom
update-monaco-vscode-1.76

Conversation

@kaisalmen

Copy link
Copy Markdown
Collaborator

This is still WIP.
The basic client/server handshake currently fails with: "Sending document notification textDocument/didOpen failed" (worker or node server same behaviour.

@CGNonofr is it possible that using this['_syncedDocuments'] for example use in DidOpenTextDocumentFeature and others no longer works with the new version?

@CGNonofr

Copy link
Copy Markdown
Collaborator

@CGNonofr is it possible that using this['_syncedDocuments'] for example use in DidOpenTextDocumentFeature and others no longer works with the new version?

I'm not sure to understand your question, what are you referring to?

@kaisalmen

kaisalmen commented Mar 22, 2023

Copy link
Copy Markdown
Collaborator Author

@CGNonofr sorry that was confusing. It was already late. 😊

What I suddenly observe is here:
https://github.com/microsoft/vscode-languageserver-node/blob/main/client/src/common/client.ts#L807
is that _didChangeTextDocumentFeature of the BaseLanguageClient is undefined. This is inside the BaseLanguageClient and I am currently clueless why this is happening suddenly. I had the impression it was related to the changes in registerBuiltinFeatures (therefore the this['_syncedDocuments']), but that's not it.
If you have an idea, I appreciate it.

@CGNonofr

Copy link
Copy Markdown
Collaborator

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

Parameters were added, we probably need to resynchronize with

👍 I suspected something like that, thanks, I will have a look. Looking where to tackle such problems is the questions sometimes.

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr have it running again locally, but need to check align/check changes

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr it is running again (worker and node server examples). I have moved things that should not be registered as default into registerNotUsedFeatures. Does this make sense to you?

@CGNonofr

Copy link
Copy Markdown
Collaborator

@CGNonofr it is running again (worker and node server examples). I have moved things that should not be registered as default into registerNotUsedFeatures. Does this make sense to you?

Sure why not, even If I don't really know who will need them (:

btw, I'm not sure why you added back CallHierarchyFeature, TypeHierarchyFeature and InlineValueFeature

@kaisalmen
kaisalmen force-pushed the update-monaco-vscode-1.76 branch from 5a847a6 to 8d0d5ff Compare March 22, 2023 18:55
@kaisalmen

Copy link
Copy Markdown
Collaborator Author

btw, I'm not sure why you added back CallHierarchyFeature, TypeHierarchyFeature and InlineValueFeature

That was a mistake. I forced pushed an update.

@CGNonofr

Copy link
Copy Markdown
Collaborator

btw, I'm not sure why you added back CallHierarchyFeature, TypeHierarchyFeature and InlineValueFeature

That was a mistake. I forced pushed an update.

I wouldn't say the ConfigurationFeature and SyncConfigurationFeature are not used either, they are just optional as it only works it you enable to configuration services on monaco-vscode-api

@kaisalmen
kaisalmen marked this pull request as ready for review March 22, 2023 19:11
@kaisalmen
kaisalmen requested a review from CGNonofr as a code owner March 22, 2023 19:11
@kaisalmen

Copy link
Copy Markdown
Collaborator Author

they are just optional as it only works it you enable to configuration services on monaco-vscode-api

ok, then they should be registered at least. The question is if there is harm done if we just use the existing method of BaseLanguageClient and register everything even if it does not make sense in the context of MonacoLanguageClient. WDYT?

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr I updated to monaco-vscode-api@1.76.2 and now the direct inclusion onig.wasm makes problems (changes to getTextmateServiceOverride):
image
image

Using vite-plugin-wasm makes things worse, that's why I disabled it again currently

If you could allow the manual loading available again (optionally), then users of the library could define the wasm loading as required. But maybe you have another idea.

@CGNonofr

Copy link
Copy Markdown
Collaborator

they are just optional as it only works it you enable to configuration services on monaco-vscode-api

ok, then they should be registered at least. The question is if there is harm done if we just use the existing method of BaseLanguageClient and register everything even if it does not make sense in the context of MonacoLanguageClient. WDYT?

It's just that the client will announce it supports features it doesn't really support, it's not ideal

@CGNonofr I updated to monaco-vscode-api@1.76.2 and now the direct inclusion onig.wasm makes problems (changes to getTextmateServiceOverride):

Using vite-plugin-wasm makes things worse, that's why I disabled it again currently

Yeah it expect the wasm to be loaded as an url, vite-plugin-wasm probably loads the code from the wasm, we don't want that (vscode-oniguruma should do it)

If you could allow the manual loading available again (optionally), then users of the library could define the wasm loading as required. But maybe you have another idea.

I hesitated making it optional, it's just that it's a hack. We can put it back. There is others assets (audio cues mp3 and some others may come later ; like the macbook touchbar icons images)

Feel free to issue a PR restoring that parameter as optional if you wish

Btw to fix it here, you can put assetsInclude: ['**/*.wasm'] in the vite config

@kaisalmen
kaisalmen force-pushed the update-monaco-vscode-1.76 branch from 5e4c665 to 9b1a2f2 Compare March 23, 2023 08:32
@kaisalmen

Copy link
Copy Markdown
Collaborator Author

It's just that the client will announce it supports features it doesn't really support, it's not ideal

Agreed. I put back ConfigurationFeature and SyncConfigurationFeature. Everything unsupported is listed in registerNotUsedFeatures which is never called.

Btw to fix it here, you can put assetsInclude: ['**/*.wasm'] in the vite config

This leads to a cors issue. I tried multiple config changes in vite, put nothing works. There must be a better solution for this. Can you inline the wasm directly when you build the library? Problem is if people need to fiddle around with tweaking bundlers (webpack fails without changes as well, btw) this will lead to adaptation issues of monaco-vscode-api.

image

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr I opened CodinGame/monaco-vscode-api#85 to discuss potential solutions to the problem above ⬆️

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr This is ready now. Do you have time to test it, especially the new Langium example?

@CGNonofr CGNonofr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG;TM

Comment thread packages/client/src/console-window.ts Outdated
@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr Do you see anything blocking the 5.0.0 release? I am in the process of updating README and CHANGELOGs

@CGNonofr

CGNonofr commented Apr 4, 2023

Copy link
Copy Markdown
Collaborator

I'm still working on some improvements of the api but I don't think it will require a major version here (even though it will change the recommended way of creating models) so let's release the 5.0

@kaisalmen

kaisalmen commented Apr 4, 2023

Copy link
Copy Markdown
Collaborator Author

Then we release to 5.1 and the "jump" will be smaller than now.
I want to get it out of the door. 😎 Last official release was in 2022.

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.

2 participants