CodinGame / monaco-vscode-api

VSCode public API plugged on the monaco editor
MIT License
214 stars 29 forks source link

Include monaco-editor, remove it from dependencies #327

Closed CGNonofr closed 5 months ago

CGNonofr commented 6 months ago

Related to https://github.com/CodinGame/monaco-vscode-api/discussions/325

@kaisalmen here's the POC

I'd like to know what you think about it

and here's a draft of what I plan to put in the release description:


Description

Remove dependency to monaco-editor: monaco-editor is now bundled by the lib.

A @codingame/monaco-vscode-editor-api package now exists to access the monaco-editor api

Migration guide:


CGNonofr commented 6 months ago

Great work! 🚀 I will test the preview version with monaco-languageclient and monaco-editor-wrapper and let you know of any issues I may encounter.

Thank you! :)

We should consider a couple of things:

  • We should gather feedback from users of monaco-vscode-api. @CompuIves your feedback will be very valuable here, I think.

Totally agree!

  • Should we ask users to extensively test the preview version before making the official release?

I don't know, it will release a major version so... anyone having issues with it can just not upgrade for the moment

  • Users of @codingame/monaco-vscode-api and monaco-languageclient should have been already aware they got/needed an altered monaco-editor and therefore should be just fine with the change. Are we overlooking something big here or can we just state: If you want to use this library stack that is the way it is!?

I don't think it complexifies the usage and I'm pretty convinced this is 99% backward compatible (1% being the internal monaco-editor module import - and we will able to fix it later module per module). I hope we don't overlook anything but I really don't know what it could be that we won't be able to fix

kaisalmen commented 6 months ago

@CGNonofr encountered a first issue: main vscode package does not export the editor in its package.json. It works in the demo, but not with the npm package:

image

CGNonofr commented 6 months ago

@CGNonofr encountered a first issue: main vscode package does not export the editor in its package.json. It works in the demo, but not with the npm package:

image

Ok thanks, there was indeed an issue, should be fixed by f95b460fd1f5ead7a54704f65a22a6758e5e748c, testing...

CGNonofr commented 6 months ago

Ok @kaisalmen it seems we SHOULDN'T rewrite the history of a PR or semantic release will fail to release another next version from the branch... (and I didn't find a way to recover it)

So I've currently publishing from another branch: v2 so the deployed version will be 2.0.0-v2.1

Can you give it a try?

kaisalmen commented 6 months ago

Can you give it a try?

It works, see https://github.com/TypeFox/monaco-languageclient/pull/599

CGNonofr commented 5 months ago

Can you give it a try?

It works, see TypeFox/monaco-languageclient#599

Ok, so what's the next step?

kaisalmen commented 5 months ago

Ok, so what's the next step?

We should merge this PR and https://github.com/TypeFox/monaco-languageclient/pull/599 . I am currently moving the wrapper end merging the example into the monaco-languageclient While doing this I will see if there are further issues. But, I don't know if I can make it this week.

We should either let other people test the pre-release, gather feedback and then release the final version at some point OR you can also release the final version soon as it is really a distinguishable major version and we report and solve potential issues later and release updates. What do you prefer?

CGNonofr commented 5 months ago

Ok, so what's the next step?

We should merge this PR and TypeFox/monaco-languageclient#599 . I am currently moving the wrapper end merging the example into the monaco-languageclient While doing this I will see if there are further issues. But, I don't know if I can make it this week.

We should either let other people test the pre-release, gather feedback and then release the final version at some point OR you can also release the final version soon as it is really a distinguishable major version and we report and solve potential issues later and release updates. What do you prefer?

The issue I see with no publishing a final version is... what do we do on bug reports and contributions? Will we stack PRs on top of each others? I think I prefer to move fast, break things (big changes will eventually have an end)

kaisalmen commented 5 months ago

I think I prefer to move fast, break things (big changes will eventually have an end)

With regard to monaco-languageclient this major version will only become relevant once version 8.0.0 is released and it will likely not be this week. More likely end of next week.

I guess you / Codingame is the major user, so will immediately notice something is broken. Others should be aware that switching to a new major version may break things.

CGNonofr commented 5 months ago

I think I prefer to move fast, break things (big changes will eventually have an end)

With regard to monaco-languageclient this major version will only become relevant once version 8.0.0 is released and it will likely not be this week. More likely end of next week.

I guess you / Codingame is the major user, so will immediately notice something is broken. Others should be aware that switching to a new major version may break things.

Exactly!

But there is so many ways to use the lib and so many features that it's really hard to guarantee no regression...

Also I think we're reactive enough on bug reports

kaisalmen commented 5 months ago

Also I think we're reactive enough on bug reports

💯

github-actions[bot] commented 5 months ago

:tada: This PR is included in version 2.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: