Skip to content

Merge monaco-editor-wrapper and monaco-editor-react into monaco-languageclient's npm workspace #600

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 16 commits into from
Mar 18, 2024

Conversation

kaisalmen
Copy link
Collaborator

@kaisalmen kaisalmen commented Jan 29, 2024

This PR is now ready for review. A couple of open points should be fixed, but review can start now.

This repository now hosts the following npm packages:

Important points:

  • The examples of this repository and the ones from https://github.com/TypeFox/monaco-components have been merged.
  • Verification examples have been removed from the npm workspace and are now contained in the directory verify.
    • The Angular example has been integrated here. The build pipeline verifies they are properly building
    • They have been excluded from main workspace to prevent pollution of the main node_modules (e.g. angular)
  • monaco-editor-workers is no longer needed and is replaced by monaco-editor-wrapper/workerFactory that integrates ideas from monaco-vscode-api demo and is fully customizable/over-writeable.
  • Client examples now make use of monaco-editor-wrapper or @typefox/monaco-editor-react. This made reduces the amount of boiler plate code.

Left over points required at least required to be implemented before this PR can be merged:

  • Browser example should also use the wrapper
  • Update all READMEs and linked documentation files
  • Check LICENSE files and statements
  • Update CHANGELOGs
  • Check if existing bugs should be fixed along or if they should be fixed with a bug-fix release
  • The worker based extension host needs to be integrate to allow advanced TypeScript support where needed in the example (wrapperTs and wrapperAdvanced), but this can be deferred. Moved to Enhance TypeScript related examples #611

Will close #543

@kaisalmen kaisalmen force-pushed the merge-monaco-edtior-wrapper branch from e7a2a44 to ee117e4 Compare February 7, 2024 09:06
@kaisalmen kaisalmen force-pushed the merge-monaco-edtior-wrapper branch from ee117e4 to 0354fe3 Compare February 14, 2024 22:05
@kaisalmen kaisalmen force-pushed the merge-monaco-edtior-wrapper branch 2 times, most recently from 9f51b4f to 816c4fa Compare February 19, 2024 16:50
@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Feb 19, 2024

@CGNonofr I have updated to 2.2.0-next.1 and the font in the command palette is wrong. Should this work without change?
image

@codingame/monaco-vscode-typescript-basics-default-extension gives me the following error:
image

and @codingame/monaco-vscode-typescript-language-features-default-extension and throws an error:
image

@kaisalmen
Copy link
Collaborator Author

Did I miss changes in the vite configuration? I just checked the your demo config and I didn't see something obvious

@CGNonofr
Copy link
Collaborator

Did I miss changes in the vite configuration? I just checked the your demo config and I didn't see something obvious

I'm not aware of anything 🤔

The first error looks like the theme service override is not registered

The second one is probably due to the webworker extension host not working (the local process extension host is not able to replace worker urls by real urls)

@kaisalmen
Copy link
Collaborator Author

I'm not aware of anything 🤔

It is consistently using the wrong font. Did you remove to many global css instructions? Interestingly, codicon is fine. In the past, usually problems originated from there.

The first error looks like the theme service override is not registered

I mixed the classical editor mode of the wrapper with the vscode extensions and therefore the themes override was not loaded. My fault.

The second one is probably due to the webworker extension host not working (the local process extension host is not able to replace worker urls by real urls)

You have a workaround for that problem in the demo, don't you? Could we generalize that somehow. I need to fix the editor worker issue as well, too.

@CGNonofr
Copy link
Collaborator

It is consistently using the wrong font. Did you remove to many global css instructions? Interestingly, codicon is fine. In the past, usually problems originated from there.

There were some removed global style I've added back (with some tweaks to remove properties on the body and so on...)

What do you expect and what do you get?

You have a workaround for that problem in the demo, don't you? Could we generalize that somehow. I need to fix the editor worker issue as well, too.

Nop, in the demo, the default extensions run in the worker ext host

@kaisalmen
Copy link
Collaborator Author

@CGNonofr just tested again with 2.2.0-next.5. First two screenshots are from an new here (Chrom on Windows). Font in command palette is with serif. The last shot is from the demo of your latest code (Chrome on Ubuntu). Font is as expected.

New example Langium Grammar DSL Language Client & Server (Worker) (Chrome on Windows):
image
image
monaco-vscode-api demo (Chrome on Ubuntu):
image

@kaisalmen
Copy link
Collaborator Author

Latest force-push using @codingame/[email protected] fixes the issue above ⬆️

@kaisalmen kaisalmen force-pushed the merge-monaco-edtior-wrapper branch 3 times, most recently from 4aef2fb to 2e44d6f Compare February 27, 2024 07:09
@kaisalmen
Copy link
Collaborator Author

@CGNonofr I am continuing to merge and update the examples (latest code is not yet pushed). Have you seen this error already? Using the standalone editor here with 2.2.1:
image

@kaisalmen
Copy link
Collaborator Author

I am not importing any workbench related things and I guess that should not be needed.

@kaisalmen kaisalmen force-pushed the merge-monaco-edtior-wrapper branch 2 times, most recently from cba8b08 to 2af7bdd Compare February 28, 2024 15:10
@ls-infra
Copy link
Contributor

should i put this #595 feature on hold until this WIP PR is merged and released ?
btw, for the language-client-runner , is there a way to save the edited value in code editor back so that it can submit back to the API? it is related to TypeFox/monaco-languageclient-ng-example#5 , i am keen to help to update the angular demo once this two way data binding feature is in place in the wrapper .

thx

@kaisalmen
Copy link
Collaborator Author

should i put this #595 feature on hold until this WIP PR is merged and released ?

Yes, that makes sense. I hope to get this ready finally this week. My own availability will be better again now.

btw, for the language-client-runner , is there a way to save the edited value in code editor back so that it can submit back to the API? it is related to TypeFox/monaco-languageclient-ng-example#5 , i am keen to help to update the angular demo once this two way data binding feature is in place in the wrapper .

The wrapper allows you already to get access to the editor/model itself. See for example. https://github.com/TypeFox/monaco-languageclient/blob/merge-monaco-edtior-wrapper/packages/wrapper/src/wrapper.ts#L156-L190 This is also put to use in some of the examples.

@kaisalmen
Copy link
Collaborator Author

@CGNonofr README updates and LICENSE unification are done. CHANGELOGs are next.

@kaisalmen
Copy link
Collaborator Author

@CGNonofr this is ready for review. Please go ahead.

@CGNonofr
Copy link
Collaborator

@CGNonofr this is ready for review. Please go ahead.

There is still a WIP commit, am I supposed to review it?

@kaisalmen
Copy link
Collaborator Author

There is still a WIP commit, am I supposed to review it?

A the first one. Nevermind, go ahead

@kaisalmen
Copy link
Collaborator Author

... or give me a 30 minutes. I can fix the commit message, but need to finish something else first

Copy link
Collaborator

@CGNonofr CGNonofr left a comment

Choose a reason for hiding this comment

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

didn't spot anything weird but not the easiest PR of my life !

…nify examples

- Updated to monaco-vscode-api 2.2.1
- Integrated python wrapper & react wrapper
- Integrated json wrapper client web socket example
- Converted groovy example to wrapper
- Updated versions to 8.0.0-next.0, 4.0.0-next.0 and 3.0.0-next.0
…nstruction of worker

- Integrate angular example
- Include build of all verification examples
- workerFactory allows to use configuration or worker for the definition
… monaco-vscode-api 3.1.2, use monaco-editor as dependency designator

- Moved some code back from the wrapper to the languageclient as otherwise it can be used standalone
- Cleaned enhanced MonacoEnvironment and fixed init bug in new common init function
@kaisalmen kaisalmen force-pushed the merge-monaco-edtior-wrapper branch from 3118e93 to b81ff04 Compare March 18, 2024 17:32
@kaisalmen
Copy link
Collaborator Author

@CGNonofr It's rebased and the first to commits on the branch have been merged.

@CGNonofr CGNonofr self-requested a review March 18, 2024 17:34
@kaisalmen
Copy link
Collaborator Author

didn't spot anything weird but not the easiest PR of my life !

Yes, I know and I am sorry for this, but merging this all in one place couldn't be done in small steps. We can iterate from here.

@kaisalmen kaisalmen merged commit 4624c5c into main Mar 18, 2024
@kaisalmen kaisalmen deleted the merge-monaco-edtior-wrapper branch March 18, 2024 20:16
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.

Is monaco-languageclient still needed in the future?
4 participants