Skip to content

Adapt to monaco-vscode-api-1.79.1 - #501

Merged
kaisalmen merged 3 commits into
mainfrom
mva-1.79.1
Jun 12, 2023
Merged

Adapt to monaco-vscode-api-1.79.1#501
kaisalmen merged 3 commits into
mainfrom
mva-1.79.1

Conversation

@kaisalmen

Copy link
Copy Markdown
Collaborator

@CGNonofr I have a problem even if I only include files and model. Any idea on this?
image

I changed initServices that users can only include editor or views as you outlined in the monaco-vscode-api README

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

Is it possible I import things from monaco-editor suddently twice?

@CGNonofr

CGNonofr commented Jun 6, 2023

Copy link
Copy Markdown
Collaborator

monaco-vscode-api includes the reference search from vscode
so monaco-editor/esm/vs/editor/standalone/browser/referenceSearch/standaloneReferenceSearch shouldn't be included, nor directly monaco-editor obviously

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

This means importing:

import 'monaco-editor/esm/vs/editor/edcore.main.js'

is now forbidden

@CGNonofr

CGNonofr commented Jun 6, 2023

Copy link
Copy Markdown
Collaborator

It seems so!

btw, it was already a bad idea, as most of them are not functionnal, like toggleHighContrast and inspectTokens.

At least now we can't make the mistake or importing it (:

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

I have forced pushed another update and checked that all examples are working fine again.
One important finding: If you use quickaccess then keybindings must be loaded otherwise triggering the command palette does not work. An error is written to the console. I handle this in the init here, but maybe something you should already handle on monaco-vscode-api level.

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

This is ready. I just released: https://www.npmjs.com/package/monaco-languageclient/v/6.1.0-next.1

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

@CGNonofr do want to change something in monaco-vscode-api regarding quickaccess and keybindings and we should wait for it or is this PR ok as is?

@CGNonofr

CGNonofr commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator

I don't know it it's a good idea to implicitly includes other services (keybindings from quickaccess)

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

I don't know it it's a good idea to implicitly includes other services (keybindings from quickaccess)

At least better than not working. Do you prefer throwing errors suggesting what to do?

@CGNonofr

CGNonofr commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator

I don't know it it's a good idea to implicitly includes other services (keybindings from quickaccess)

At least better than not working. Do you prefer throwing errors suggesting what to do?

I think it's preferable

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

I am 50/50 on that, but with more services (I have seen the next branch with more services 😱 😆 ) and growing complexity it might be a better idea.

@kaisalmen

kaisalmen commented Jun 8, 2023

Copy link
Copy Markdown
Collaborator Author

@CGNonofr now errors are thrown, if dependent services are not configured. I kept the files service being always loaded. Any further objections?

@CGNonofr

CGNonofr commented Jun 8, 2023

Copy link
Copy Markdown
Collaborator

now errors are thrown

you mean no?

@kaisalmen

Copy link
Copy Markdown
Collaborator Author

Sorry I corrected the sentence above: "now errors are thrown, if dependent services are not configured"

Comment thread vite.config.ts Outdated
@kaisalmen
kaisalmen merged commit 4c08ba7 into main Jun 12, 2023
@kaisalmen
kaisalmen deleted the mva-1.79.1 branch June 12, 2023 07:04
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