-
Notifications
You must be signed in to change notification settings - Fork 317
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
Start Ydoc with the language server #9862
Conversation
4acf4d6
to
22488da
Compare
// ydoc-server | ||
requires io.helidon.webclient; | ||
requires io.helidon.webclient.websocket; | ||
requires io.helidon.webserver; | ||
requires io.helidon.webserver.websocket; |
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.
Good solution for now, if everything works with that. But at the end, we must ensure that runtime.jar
does not require helidon modules. Let's keep that in mind and create a separate ticket for that before merging.
@@ -13,9 +13,9 @@ public class ClassLoaderConstants { | |||
* consistent. | |||
*/ | |||
public static final List<String> CLASS_DELEGATION_PATTERNS = | |||
List.of("org.graalvm", "java", "org.slf4j", "ch.qos"); | |||
List.of("org.graalvm", "java", "org.slf4j", "ch.qos", "io.helidon"); |
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.
Good. This ensures that our boot class loader deleagates loading of all io.helidon.*
classes to the system class loader, which picks the classes from JARs inside the component
directory.
5958a94
to
d77d8a5
Compare
d0f2108
to
8a16bf9
Compare
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.
It is great to see this to be green. Let's integrate and let's unblock the cloud integration.
hey, just wondering - what's the roadmap for this stuff? will this be theoretically ready for integration with Cloud after this PR? or are there more steps that are needed? |
This should be enough to start integration in the cloud |
@Frizi waiting for your approval |
| update: Ydoc starts with the language server on the localhost:1234 by default. The hostname and ports can be configured by setting environment variables LANGUAGE_SERVER_YDOC_HOSTNAME and LANGUAGE_SERVER_YDOC_PORT Can you The port should be fixed there and both |
Sure, I'll update the Docerfile in the follow-up PR. This one has already too many stuff in it |
} | ||
try (var ydoc = new Ydoc.Builder().build()) { | ||
ydoc.start(); | ||
lock.acquire(); |
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.
Is it intentional that this Semaphore is never released?
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.
yes, we are waiting for any interruption signal here. Otherwise, the main process ends.
I realise that once this was merged, I can no longer compile the engine through sbt in IntelliJ, because I use the (recommended) version manager for Node: For now I just need to switch to using sbt from within PowerShell. This is okay, but a bit annoying as IntelliJ will start the sbt shell anyway to re-import the project from time to time. Has anyone had a chance to set up IntelliJ to pick up the Node paths from |
@radeusgd My IntelliJ's builtin sbt shell stopped working approx 9 months ago. I have switched to a separate |
Pull Request Description
Changelog:
localhost:1234
by default. The hostname and ports can be configured by setting environment variablesLANGUAGE_SERVER_YDOC_HOSTNAME
andLANGUAGE_SERVER_YDOC_PORT
npm dev run
uses the node Ydoc server. You can control it withPOLYGLOT_YDOC_SERVER
env variable. For example,⠀
npm install
before the engine build. It is required to create the Ydoc JS bundle.Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.