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

Improve code splitting #416

Merged
merged 12 commits into from
Apr 29, 2024
Merged

Improve code splitting #416

merged 12 commits into from
Apr 29, 2024

Conversation

CGNonofr
Copy link
Contributor

@CGNonofr CGNonofr commented Apr 26, 2024

Extract the service identifiers into their own files

Many services (especially those not browser or native specific) have their implementation in the same file as the service identifier, making it impossible to get the service identifier without the implementation.

In monaco-vscode-api, we import all service identifiers from missing-services.ts to create an empty overridable implementation of them, currently leading to importing the VSCode implementation as well, making the bundle bigger is the treeshaker is not able to remove them, and making the library tarball bigger anyway

Also update to monaco-editor 0.48 which was released in the meantime but there is no change expected

@github-actions github-actions bot temporarily deployed to commit April 26, 2024 12:39 Inactive
@CGNonofr CGNonofr mentioned this pull request Apr 26, 2024
@CGNonofr
Copy link
Contributor Author

@kaisalmen 1610bf3 may impact your monaco-editor-wrapper, you may want to import the new monarch service override when textmate is not used

Related to microsoft/monaco-editor#4463

@kaisalmen
Copy link
Collaborator

Related to microsoft/monaco-editor#4463

@CGNonofr Thank you, for seeing that. And thank you for providing the additional service override.

I am building this branch locally right now and will test it today. This is a big change, but welcome one. Do you want to create another pre-release or do you want to release a new version soon (as soon as I approve)? What do you prefer?

As usual I will test this with the wrapper and the all examples in the mlc repo and provide feedback. From my point of view we can directly release new version. I also wanted to implement another improvement in the wrapper, but did not had the time, yet.

@CGNonofr
Copy link
Contributor Author

Related to microsoft/monaco-editor#4463

@CGNonofr Thank you, for seeing that. And thank you for providing the additional service override.

I am building this branch locally right now and will test it today. This is a big change, but welcome one. Do you want to create another pre-release or do you want to release a new version soon (as soon as I approve)? What do you prefer?

As usual I will test this with the wrapper and the all examples in the mlc repo and provide feedback. From my point of view we can directly release new version. I also wanted to implement another improvement in the wrapper, but did not had the time, yet.

I like to move fast and break things!

It will be a new minor version, anyone encountering issues can stay on the previous release

Copy link
Collaborator

@kaisalmen kaisalmen left a comment

Choose a reason for hiding this comment

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

LGTM. Will test this with the usual suspects once it released.

@CGNonofr CGNonofr merged commit d104adf into main Apr 29, 2024
2 checks passed
@CGNonofr CGNonofr deleted the improve-code-splitting branch April 29, 2024 12:09
Copy link

🎉 This PR is included in version 4.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@kaisalmen
Copy link
Collaborator

@CGNonofr I needed to some extra packages otherwise vite dev server complains: https://github.com/TypeFox/monaco-languageclient/pull/648/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519
image

Apart form that it looks good. I opened TypeFox/monaco-languageclient#648

@CGNonofr
Copy link
Contributor Author

well spotted...

@CGNonofr
Copy link
Contributor Author

@kaisalmen #419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants