Skip to content
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

Do not reexport code of imported libraries (e.g. vscode-languageclient) #459

Merged
merged 4 commits into from
Mar 3, 2023

Conversation

kaisalmen
Copy link
Collaborator

@kaisalmen kaisalmen commented Feb 9, 2023

This PR ensure that monaco-languageclient and vscode-ws-jsonrpc no longer export imported packages.

Additional changes:

  • The browser-lsp example is replaced with langium statemachine example. No more copied code from Microsoft here.
    • It also makes use of multiple services from monaco-vscode-api
  • Examples have been moved to one packages with the exception of angular due to its own build approach
  • There now is a global watch available that watches code in all packages (except for angular and verify where this strategy cannot be applied).
  • The usual dependency updates
  • node 18 / npm 9 is now configured for Volta
  • I am preparing vitest inclusion (if you wonder why there now is tsconfig.src.json)

I will release next packages to test the implementation and make this final once I think the PR is ready!

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Feb 13, 2023

@CGNonofr This is not complete, but the new example may be of interest for you. Are the vscode default themes available via some npm package? Do you know this by any chance?

@CGNonofr
Copy link
Collaborator

CGNonofr commented Feb 13, 2023

@CGNonofr This is not complete, but the new example may be of interest for you.

Interesting! I'll have a look

Are the vscode default themes available via some npm package? Do you know this by any chance?

Unfortunately no... they are only available as a vscode extension. I had to download the files by hands (So I guess the answer is yes are they are available in our monaco editor wrapper? :)

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Feb 24, 2023

Fixed some things and added a node script for fetching themes from code's GH repo.
Themes are not committed, but one sneaked in. Force Push ⬇️

@kaisalmen kaisalmen force-pushed the issue-447 branch 2 times, most recently from 54f87fc to 29e8206 Compare February 27, 2023 13:07
@kaisalmen kaisalmen marked this pull request as ready for review February 27, 2023 16:33
@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Feb 27, 2023

@CGNonofr this is now ready for review.
I have removed the disposable.ts from monaco-languageclient and only left it in vscode-ws-jsonrpc.
Documentation (README/CHANGELOG has been updated)
I have integrated vscode themes helper into monaco-languageclient as a separate export. Maybe this can be even moved to monaco-vscode-api

@kaisalmen kaisalmen force-pushed the issue-447 branch 2 times, most recently from 1512994 to a96f797 Compare March 3, 2023 08:30
@kaisalmen
Copy link
Collaborator Author

@CGNonofr This PR exploded in size. Sorry.
Combing our examples (with exception of angular, webpack and vite verification) reduces maintenance effort.
Do you have time to look at this?

@CGNonofr
Copy link
Collaborator

CGNonofr commented Mar 3, 2023

@CGNonofr This PR exploded in size. Sorry. Combing our examples (with exception of angular, webpack and vite verification) reduces maintenance effort. Do you have time to look at this?

I have a deal then: will you have time to handle to update of monaco-vscode-api to monaco@0.36?

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.

hard to review indeed!

LGTM, I just don't know if integrating theme management in monaco-languageclient is relevant

@kaisalmen
Copy link
Collaborator Author

I have a deal then: will you have time to handle to update of monaco-vscode-api to monaco@0.36?

I actually would like to understand how you do this whole transformation. My time next week is very limited, though.

LGTM, I just don't know if integrating theme management in monaco-languageclient is relevant

I will make up my mind again. I will eventually move it to our monaco-editor-wrapper and have some stripped down version here. Do you think it makes sense to have a "theme helper" in monaco-vscode-api?

@CGNonofr
Copy link
Collaborator

CGNonofr commented Mar 3, 2023

I actually would like to understand how you do this whole transformation. My time next week is very limited, though.

pretty straighforward:
1 - update monaco-editor and config.vscode.version in package.json, it will download the proper vscode version
2 - fix compilation errors (by updating the code and vscode.patch if required)
3 - test it and fix runtime error (probably caused by a missing polyfill)

I will make up my mind again. I will eventually move it to our monaco-editor-wrapper and have some stripped down version here. Do you think it makes sense to have a "theme helper" in monaco-vscode-api?

Maybe we can have something more generic which allows to load a vscode extension instead from a github url?

@kaisalmen
Copy link
Collaborator Author

Maybe we can have something more generic which allows to load a vscode extension instead from a github url?

Sounds like a good idea. Load and use (does monac-vscode-api support or are you already doing it elsewhere?) or load and unpack and use what's needed.

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Mar 3, 2023

Btw, I have published a couple of pre-release of 5.0.0 so far (current next is is 5.0.0-next.3). I don't see the v5 immediately after this merge. We should wait for next monaco-vscode-api 0.36.0 IMO.

@kaisalmen kaisalmen merged commit 47264b5 into main Mar 3, 2023
@kaisalmen kaisalmen deleted the issue-447 branch March 3, 2023 13:11
@CGNonofr
Copy link
Collaborator

CGNonofr commented Mar 3, 2023

Sounds like a good idea. Load and use (does monac-vscode-api support or are you already doing it elsewhere?) or load and unpack and use what's needed.

That basically what we do in our @codingame/monaco-editor-wrapper. It probably make sense to make it more generic and put in inside monaco-vscode-api.

@kaisalmen
Copy link
Collaborator Author

It probably make sense to make it more generic and put in inside monaco-vscode-api.

This sounds like a good idea and could open a couple of more use cases 🎉

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.

None yet

2 participants