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
base: develop
Are you sure you want to change the base?
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
@@ -63,7 +59,7 @@ export default defineConfig({ | |||
...getDefines(localServerPort), | |||
IS_CLOUD_BUILD: JSON.stringify(IS_CLOUD_BUILD), | |||
PROJECT_MANAGER_URL: JSON.stringify(projectManagerUrl), | |||
YDOC_SERVER_URL: IS_POLYGLOT_YDOC_SERVER ? JSON.stringify('defined') : undefined, | |||
YDOC_SERVER_URL: JSON.stringify(POLYGLOT_YDOC_SERVER), |
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.
I guess this change is one of the causes for such a weird config check. The undefined
value should not be stringified here.
YDOC_SERVER_URL: JSON.stringify(POLYGLOT_YDOC_SERVER), | |
YDOC_SERVER_URL: POLYGLOT_YDOC_SERVER ? JSON.stringify(POLYGLOT_YDOC_SERVER) : undefined, |
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.
JSON.stringify(undefined) === undefined
anyway. I was using this special 'undefined'
string because the config only allows having string values. I refactored the logic
if (jsonAddress == null) { | ||
toastAndLog('noJSONEndpointError') | ||
} else if (binaryAddress == null) { | ||
toastAndLog('noBinaryEndpointError') | ||
} else { | ||
let ydocAddress: string | ||
if (ydocUrl == null) { | ||
ydocAddress = 'undefined' |
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.
Again unexpected stringified undefined
that I'd like to get rid of.
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.
This special value can be confusing, I agree. I refactored the ydocUrl
logic
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 |
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.