-
Notifications
You must be signed in to change notification settings - Fork 33
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
Upgrade Showcase/Playgrounds Monaco React/Wrapper Usage #171
Conversation
- Fix monaco-editor loading - Update monaco-editor-react. Remove monaco-editor related init issues - Bundle monaco-editor-react as prestep - Remove copying the bundle - Remove unnecessary npx usage
@montymxb works nicely on Windows with Chrome and Firefox |
@kaisalmen not sure, but I don't believe it's new. |
4743c00
to
d0cbab3
Compare
const documentChangeNotification = new NotificationType<DocumentChange>('browser/DocumentChange'); | ||
const jsonSerializer = grammar.serializer.JsonSerializer; | ||
shared.workspace.DocumentBuilder.onBuildPhase(DocumentState.Validated, documents => { | ||
for (const document of documents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice what you have done in this file :)
I like React function components more: https://www.robinwieruch.de/react-function-component/ But it would require to rewrite some stuff. The only, reeal benefit is shorter and more readable code. |
@Lotes I would generally agree for functional components, however we have a practical constraint given we use refs to allow interaction with the wrapper contained inside the component. Apparently this isn't supported for functional components in their current form (legacy docs), but it may be possible in newer versions, I'm just not sure how yet. |
@montymxb I checked both tutorial updates and you did a great job! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things should be adjusted, otherwise this is great!
And, @Lotes already provided very useful suggestions. It is sometimes good to be the second reviewer. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superb work! Let's go. 🚀
hi, can i somewhere find a complete hello world example for the new code. |
Hi @cdietrich . Yeah the new version has a different API than before, so setting it up as it was done before won't work as expected. To assist with this, we did update the Langium and Monaco docs to reflect setting up with version 2.1.1 of the monaco-editor-wrapper (which also happens to be targeted towards MiniLogo as well). If you haven't read that since it changed, I would double check those new docs, it might help. As for the MiniLogo repo itself, it has not yet been updated to use version 2.1.1 of wrapper. We haven't gotten to that yet, but that will naturally follow. |
@montymxb yes I followed that steps but it (the layout) is not working. This is why I am asking |
I took a look at your implementation, I think the issue is that you're opting to use a classic editor config (Monarch), but you're not passing a languageDef to utilize for that config. You're registering the id, but not the actual grammar itself, which is necessary for that to work. For reference on how to set that up, you can see how we add the languageDef to our user configs for the playgrounds & showcases. You'll need a monarch grammar handy to pass in, which you can generate locally via Langium by tweaking your langium-config.json to output that in addition to a TextMate grammar when generating (although I think that's enabled by default now with the latest yeoman generator). Give that a shot, and let me know if you're still having issues after that. |
@montymxb i had left the monarch out cause we use semantic tokens anyway. there is still the white small text-area |
You're missing the necessary styles for the editor. They used to be added in via <link href="/monaco-editor-wrapper/assets/style.css" rel="stylesheet"/> The wrapper's assets are in your public root, so that should give you the styles you need, and your layout should be corrected. Checked this out locally real quick and it seems to have done the trick. We missed adding this detail in the new docs, so we need to update that as well to ensure others don't run into the same issue. |
thx. this is working. |
i also assume the yeoman generator will be adapted anyway to reflect it. |
@cdietrich I opened eclipse-langium/langium#1163 for this |
This updates the showcase & playgrounds to use new versions of the
MonacoEditorLanguageClientWrapper
&MonacoEditorReactComp
instances. The main change was the API interface for setting up the wrapper & the React component, which is composed of a more direct configuration object object, rather than several smaller configuration functions. This allowed us to have more control over the existing implementations, as well as to cut down on the configuration code for most cases.The playgrounds required a rework to be compatible with the new approach, which exposed fewer low-level details than previously available. The new playgrounds approach has 2 wrappers like before, but the orchestration between the two underlying language servers is much simpler, and there's no longer a need to interact with the low-level message readers/writers. The current orchestration is setup like follows:
I've tried to make the process as explicit as possible in the comments, but feel free to dig into things as needed to get more details. Overall, despite tearing down the 2nd DSL editor on getting a fresh grammar, the new approach is very fast. Application response times are about an order of magnitude quicker than they were before, and can probably be improved further if we really want to. An example of this can be seen below.
A few checks that have been done so far:
I haven't done any checks on Windows yet, so any insight here would be helpful to verify there's no bundle related issues.
I'm also open to suggestions, improvements, etc.!
TODO:
README needs updating to follow new usageRebasing needed, and the new showcase examples need to be upgraded as well following this